Skip to content

Conversation

@jhiapple
Copy link
Contributor

URLs can have commas which makes the URLs fragile when multiple URLs are returned, and the values of multiple X-Effective-URL headers are merged.

Follow-up on 170e12b (Adds new X-Effective-URL header to the xdebug plugin (#7931))

URLs can have commas which makes the URLs fragile when multiple URLs are returned,
and the values of multiple X-Effective-URL headers are merged.

Follow-up on 170e12b (Adds new X-Effective-URL header to the xdebug plugin (apache#7931))
@maskit maskit added the xdebug xdebug plugin label Jun 22, 2023
@maskit maskit added this to the 10.0.0 milestone Jun 22, 2023
@apache apache deleted a comment from zwoop Jun 22, 2023
@ywkaras
Copy link
Contributor

ywkaras commented Jun 22, 2023

I suggest asking on the users mailing list if this would cause problems for anyone.

@jhiapple
Copy link
Contributor Author

I suggest asking on the users mailing list if this would cause problems for anyone.

I'd be really surprised if anyone else than me is using the X-Effective-URL debug feature, which I added two years ago for a very particular use case I have.

Google search for

trafficserver "x-effective-url"

returns exactly two (2) hits, one in the 9.x "what's new" and one in the 9.2.0 changeling.

@bryancall
Copy link
Contributor

@ywkaras ywkaras self-requested a review June 26, 2023 22:24
@maskit
Copy link
Member

maskit commented Jun 26, 2023

Note: the reason I suggested to back port this change to 9.2.x is that I think this is a bug (not just an enhancement). URLs that contain commas are problematic if those are in header values. See also the example on the RFC.
https://www.rfc-editor.org/rfc/rfc9110.html#name-field-values

@ywkaras
Copy link
Contributor

ywkaras commented Jun 28, 2023

Can you add documentation for this option in doc/admin-quide/plugins/xdebug.en.rst ?

@bneradt
Copy link
Contributor

bneradt commented Jun 29, 2023

[approve ci clang-format]

@zwoop
Copy link
Contributor

zwoop commented Jul 3, 2023

Not 100% sure this should go into 9.2.x, considering it does change behavior.

@ywkaras
Copy link
Contributor

ywkaras commented Jul 6, 2023

This is the spec for a quoted string in an HTTP header field value: https://www.rfc-editor.org/rfc/rfc9110.html#name-quoted-strings

So, the only potential non-compliance would be if a double quote was in the URL (without being URL encoded as %22). Can we be sure this won't happen?

@maskit
Copy link
Member

maskit commented Jul 6, 2023

Double-quote is one of unsafe characters, and "All unsafe characters must always be encoded within a URL" according to RFC1738.
https://datatracker.ietf.org/doc/html/rfc1738#section-2.2

@ywkaras
Copy link
Contributor

ywkaras commented Jul 6, 2023

Double-quote is one of unsafe characters, and "All unsafe characters must always be encoded within a URL" according to RFC1738. https://datatracker.ietf.org/doc/html/rfc1738#section-2.2

Presumably that means ATS would ignore a request with a non-escaped double quote in a URL component. But, if not, I supposed that would be an issue to handle in a separate PR.

However, we still have the issue that this is an undocumented feature of xdebug.

@ywkaras ywkaras changed the title Add doublequotes around the URL of X-Effective-URL. For xdebug plugin, add doublequotes around the URL of X-Effective-URL. Jul 6, 2023
@jhiapple
Copy link
Contributor Author

Added documentation for the feature.

@maskit maskit merged commit 7016476 into apache:master Jul 17, 2023
@jhiapple jhiapple deleted the double-quote-url-for-x-effective-url branch July 18, 2023 06:28
masaori335 pushed a commit to masaori335/trafficserver that referenced this pull request Sep 26, 2023
apache#9899) (apache#615)

* Add doublequotes around the URL of X-Effective-URL.

URLs can have commas which makes the URLs fragile when multiple URLs are returned,
and the values of multiple X-Effective-URL headers are merged.

Follow-up on 170e12b (Adds new X-Effective-URL header to the xdebug plugin (apache#7931))

* Document the X-Effective-URL.

(cherry picked from commit 7016476)

Co-authored-by: jhiapple <85640167+jhiapple@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

xdebug xdebug plugin

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants