Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix typo #18

Closed
wants to merge 8 commits into from
Closed

Fix typo #18

wants to merge 8 commits into from

Conversation

TomMettam
Copy link

"er" is undefined - my bad!

@dblock
Copy link
Collaborator

dblock commented Jan 11, 2017

I've merged some of this via 25789e1. I don't understand the use change, can you please explain?

I've also added tests in #23. Would you be so kind to write tests for all the changes, the error one would be a great start.

@TomMettam
Copy link
Author

TomMettam commented Jan 18, 2017

5b9b35b was a security fix. A browser could provide a header like:

X-Forwarded-For: 127.0.0.1

which would allow it to bypass the alexa verification process. This change ensures that X-Forwarded-For is only observed if it's sent from a local server (such as a proxy). Otherwise, the real IP is used.

@dblock
Copy link
Collaborator

dblock commented Jan 18, 2017

That's fine, and that whole logic moved to alexa-verifier.

I was asking about this:

screen shot 2017-01-18 at 7 40 35 am

@TomMettam
Copy link
Author

Ah yes - well, that is because bodyParser.urlencoded doesn't necessary work for all the user server modules.

In my project, for example ( https://gyazo.com/bc5b88a444b8ff334b78a5771b3d80c0 ) there are many express routes added which don't use urlencoded forms, so blanket-applying bodyParser.urlencoded to every request causes problems.

@dblock dblock mentioned this pull request Jan 18, 2017
@dblock
Copy link
Collaborator

dblock commented Jan 18, 2017

Oh I see, that makes a lot of sense! I opened #35 and will close this.

If you have time, would love some tests and a pull request for that issue.

@dblock dblock closed this Jan 18, 2017
@tejashah88 tejashah88 mentioned this pull request Feb 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants