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

Referrer-Policy Header #920

Merged
merged 1 commit into from
Oct 9, 2016
Merged

Conversation

gsquire
Copy link
Contributor

@gsquire gsquire commented Oct 6, 2016

Instead of parsing one value in the ReferrerPolicy struct, allow for a list and use the last value. I believe this is what the ticket mentions, but I could have misinterpreted.

Closes #882

@seanmonstar
Copy link
Member

/cc @jdm

@jdm
Copy link

jdm commented Oct 6, 2016

This isn't quite right - this still only looks at the very last comma-separated values and ignores the others. We want to return the rightmost value that is successfully recognized - this implies that we either need to iterate left to right and track the most recent successful parse, or iterate right to left and return the first value that is successful.

@gsquire
Copy link
Contributor Author

gsquire commented Oct 6, 2016

Ah I see.

I think the ideal way to do that would be to scan right to left checking the value each time and returning the first valid one. I will make that change. Thanks for your input.

@seanmonstar
Copy link
Member

@gsquire it would also help here to include a test in this module of this behavior

@gsquire gsquire force-pushed the referrer-update branch 2 times, most recently from 8eb1797 to f518583 Compare October 6, 2016 23:46
@gsquire
Copy link
Contributor Author

gsquire commented Oct 6, 2016

I don't think my solution is elegant so I am open to suggestions on improving it.

I also rebased onto master.

@gsquire
Copy link
Contributor Author

gsquire commented Oct 8, 2016

I refactored the ugly flow I had before. It should be good to go now.

@jdm
Copy link

jdm commented Oct 8, 2016

This looks good to me!

@seanmonstar seanmonstar merged commit c2734c6 into hyperium:master Oct 9, 2016
@seanmonstar
Copy link
Member

Thanks!

@gsquire gsquire deleted the referrer-update branch October 10, 2016 06:18
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.

3 participants