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

Jetty 9.4.x 5104 incorrect via header #5144

Merged
merged 8 commits into from
Aug 13, 2020

Conversation

sbordet
Copy link
Contributor

@sbordet sbordet commented Aug 12, 2020

No description provided.

travisspencer and others added 2 commits August 12, 2020 16:55
…other protocols

Signed-off-by: Travis Spencer <travis@curity.io>
… in Via header when accessed over H2.

* Introduced HttpFields.computeField() to put/append header values.
* Reworked AbstractProxyServlet.addViaHeader().

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
@sbordet
Copy link
Contributor Author

sbordet commented Aug 12, 2020

@joakime still does not pass ECA check so it was not the newline before Signed-off-by.

For reference, we filed an issue about this ECA at Eclipse: https://bugs.eclipse.org/bugs/show_bug.cgi?id=565761

@travisspencer can you double check on your side? Maybe the ECA is expired or has been revoked?

@travisspencer
Copy link
Contributor

The ECA is fixed. The problem was on my end, and it wasn't easy to figure out. What happened was that I logged into my Eclipse account page, and my password manager was changing the email address field that I saw to the email address I was expecting to find -- travis@.... I tried to login with another browser because I wasn't able to set my GitHub ID like Christopher Guindon suggested in the Eclipse issue tracker -- it wouldn't save though. When I did that in another browser, I noticed the email was set to travis.spencer@... which didn't match the commit. I changed it in that browser and the validation succeeded.

VERY SORRY for all the trouble and thanks for the help and patience.

else
{
found = true;
HttpField newField = computeFn.apply(name, Collections.unmodifiableList(getFields(name)));
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not keen on this implementation as for the common cases it will create a bit more garbage than necessary. However I don't really want to hold up this PR any more, so perhaps I'll do a different PR to improve this implementation.... and look at using it in other places.

{
HttpField newField = computeFn.apply(name, null);
if (newField != null)
put(newField);
Copy link
Contributor

Choose a reason for hiding this comment

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

working on a better impl... meanwhile use add here not put, as you have already iterated and discovered that it is not found.

gregw and others added 6 commits August 13, 2020 10:06
Signed-off-by: Greg Wilkins <gregw@webtide.com>
… in Via header when accessed over H2.

Fixed javadocs.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
Signed-off-by: Greg Wilkins <gregw@webtide.com>
unmodifiable found list

Signed-off-by: Greg Wilkins <gregw@webtide.com>
… in Via header when accessed over H2.

Added HttpFields.computeField() for HttpHeader too and updated usages.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
@sbordet sbordet requested a review from gregw August 13, 2020 09:50
Copy link
Contributor

@gregw gregw left a comment

Choose a reason for hiding this comment

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

LGTM other than a niggle

if (viaFields == null || viaFields.isEmpty())
return new HttpField(header, viaHeaderValue);
String separator = ", ";
String newValue = viaFields.stream()
Copy link
Contributor

Choose a reason for hiding this comment

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

I trust that this fluent style will resolve to something efficient enough for the common case of a single Via field? If not then it might not be a bad idea to have special handling for viaFields.size()==1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gregw lost in the rest of the proxying it's a case where I'd prefer less code than more special case handling.
The typical case is that we are the only proxy, so viaFields.size()==0, which is already optimized.

@sbordet sbordet merged commit 1b3cb2e into jetty-9.4.x Aug 13, 2020
@sbordet sbordet deleted the jetty-9.4.x-5104-incorrect_via_header branch August 13, 2020 12:23
@joakime
Copy link
Contributor

joakime commented Aug 13, 2020

The ECA is fixed. The problem was on my end, and it wasn't easy to figure out. What happened was that I logged into my Eclipse account page, and my password manager was changing the email address field that I saw to the email address I was expecting to find -- travis@.... I tried to login with another browser because I wasn't able to set my GitHub ID like Christopher Guindon suggested in the Eclipse issue tracker -- it wouldn't save though. When I did that in another browser, I noticed the email was set to travis.spencer@... which didn't match the commit. I changed it in that browser and the validation succeeded.

VERY SORRY for all the trouble and thanks for the help and patience.

Ah! that now makes sense.

So I opened an issue to get accounts.eclipse.org to fix this behavior - https://bugs.eclipse.org/bugs/show_bug.cgi?id=566041

@travisspencer
Copy link
Contributor

So I opened an issue to get accounts.eclipse.org to fix this behavior - https://bugs.eclipse.org/bugs/show_bug.cgi?id=566041

What you suggest there would have helped a lot.

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.

4 participants