Skip to content
This repository has been archived by the owner on Dec 12, 2022. It is now read-only.

Fix security issue (#61) #62

Merged
merged 2 commits into from
Oct 11, 2018

Conversation

arpit2297
Copy link

@arpit2297 arpit2297 commented Oct 3, 2018

Added a method to check for malicious inputs

Issue: #61

Added a test to check for malicious input to test the sanitize_path function

@DEGoodmanWilson
Copy link
Owner

Aw, this is great, thank you! Would you mind adding some testing around this as well?

@DEGoodmanWilson
Copy link
Owner

Also, it looks like your changes are causing tests to fail 😢

@arpit2297 arpit2297 force-pushed the stable/5.0.2 branch 2 times, most recently from cdbfa2f to e27e967 Compare October 5, 2018 07:40
@arpit2297
Copy link
Author

I'm not able to get the build setup. I'm running the server on a docker container using the following commands:

docker build -t awesomesauce .
docker run -p 8080:80 awesomesauce

which tells me that the server is running on port 80, with localhost (0.0.0.0) forwarding connections from port 8080 to 80 on the server. When I try

curl -v http://localhost:8080/static/main.cpp

I get no response from the server.

*   Trying ::1...
* TCP_NODELAY set
* Connected to localhost (::1) port 8080 (#0)
> GET /static/main.cpp HTTP/1.1
> Host: localhost:8080
> User-Agent: curl/7.54.0
> Accept: */*
> 
* Empty reply from server
* Connection #0 to host localhost left intact
curl: (52) Empty reply from server

@DEGoodmanWilson
Copy link
Owner

DEGoodmanWilson commented Oct 8, 2018

That was a mistake in the documentation! I have updated the Dockerfile so that the documentation is correct ;) Take a look at the latest commits on master

The biggest change is that the correct command is in fact
docker run -p 8080:8080 awesomesauce

@arpit2297 arpit2297 force-pushed the stable/5.0.2 branch 3 times, most recently from aae7826 to a2d7c62 Compare October 9, 2018 07:41
@DEGoodmanWilson DEGoodmanWilson changed the base branch from stable/5.0.2 to master October 9, 2018 08:13
@DEGoodmanWilson
Copy link
Owner

Sorry, small favor to ask because I changed something in the way that the branches are organized. Would you mind rebasing from DEGoodmanWilson:master?

Copy link
Owner

@DEGoodmanWilson DEGoodmanWilson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking really great, thank you for your hard work! I found a place where we can optimize the reconstruction of the sanitized path from the stack—see the review details. Also—and this is absolutely my fault—can you rebase from DEGoodmanWilson:master? I'm reorganizing how the branches work, and trying to return to a mode where mainline development is performed on master, rather than starting from the last release.

luna/router.cpp Outdated

// Build final path from stack
while (not stk.empty()) {
url_comps.push_back(stk.top());
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From line 103 forward, we can simplify:

    // Build final path from stack
    while (not stk.empty())
    {
        final_path = stk.top()+delimeter+final_path;
        stk.pop();
    }

    // remove trailing '/'
    final_path.pop_back();

    return final_path;

tests/file_service.cpp Show resolved Hide resolved
@arpit2297
Copy link
Author

arpit2297 commented Oct 10, 2018

Made the changes, and also rebased from your master branch as well. Let me know if its good to merge in! :)

Copy link
Owner

@DEGoodmanWilson DEGoodmanWilson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again for being so patient with this review. I'm really excited that you've chipped in! I've left a comment on your latest revisions, which are quite thoughtful. But I'm going to approve the PR because a) you've written the key tests and b) they pass. Everything else is icing at this point, so if you want to address the remaining comment, feel free, otherwise I will merge this in tomorrow.

luna/router.cpp Show resolved Hide resolved
@arpit2297
Copy link
Author

Please merge whenever you wish to! :) Thanks for the support with this issue!

@DEGoodmanWilson DEGoodmanWilson merged commit faded80 into DEGoodmanWilson:master Oct 11, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants