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

Refactor and improve ErrorState::Dump #1730

Closed
wants to merge 39 commits into from

Conversation

kinkie
Copy link
Contributor

@kinkie kinkie commented Mar 9, 2024

Rework the internals for generating output in ErrorState::Dump,
used for expanding the '%W' token in error page templates.

Also fix a bug with excessive html-quoting of the output.

@kinkie
Copy link
Contributor Author

kinkie commented Mar 9, 2024

Run-tested. Output:

?subject=CacheErrorInfo%20-%20ERR_ACCESS_DENIED**&**body=CacheHost%3A%20n2%0D%0AErrPage%3A%20ERR_ACCESS_DENIED%0D%0AErr%3A%20%5Bnone%5D%0D%0ATimeStamp%3A%20Sat%2C%2009%20Mar%202024%2014%3A48%3A00%20GMT%0D%0AClientIP%3A%20127.0.0.1%3A53002%0D%0A%0D%0AHTTP%20Request%3A%0D%0AGET%20%2Fbar%20HTTP%2F1.1%0D%0AHost%3A%20www.fb.com%0D%0AUser-Agent%3A%20curl%2F7.81.0%0D%0AAccept%3A%20%2A%2F%2A%0D%0AProxy-Connection%3A%20Keep-Alive%0D%0A%0D%0A%0D%0A

In bold is a bug (&amp should be &).
That bug is not new, master has the same faulty behaviour

src/errorpage.cc Outdated Show resolved Hide resolved
@rousskov rousskov self-requested a review March 9, 2024 19:08
Copy link
Contributor

@rousskov rousskov left a comment

Choose a reason for hiding this comment

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

Run-tested. Output: [%W value]

Please post two %W values: one before and one after these changes. Do not use text formatting. Do use diff quoting format to improve readability:

   ```diff
   - before
   + after 
   `` `

If you do it right, you will see GitHub rendering your diff as a diff in Preview, like here:

- before
+ after

(&amp should be &). That bug is not new, master has the same faulty behaviour

Are you preserving the old bug because it is difficult to fix or for some other reason? If it is difficult to fix, can you mark the buggy code with an XXX C++ comment?

src/errorpage.cc Outdated Show resolved Hide resolved
src/errorpage.cc Show resolved Hide resolved
@rousskov rousskov added the S-waiting-for-author author action is expected (and usually required) label Mar 10, 2024
@kinkie kinkie added the S-waiting-for-PR Closure of other PR(s), current or future, is expected (and usually required) label Mar 11, 2024
@kinkie

This comment was marked as resolved.

@yadij
Copy link
Contributor

yadij commented Mar 11, 2024

?subject=CacheErrorInfo%20-%20ERR_ACCESS_DENIED**&**body=CacheHost%3A%20n2%0D%0AErrPage

In bold is a bug (&amp should be &). That bug is not new, master has the same faulty behaviour

IIRC this string is being generated for a mailto:admin@example.com?subject=...&body=... URL query-string. The particular & you have marked in bold is the query-string delimiter which should not be URL-encoded.

Copy link
Contributor

@yadij yadij left a comment

Choose a reason for hiding this comment

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

LGTM, just Alex review items to resolve.

@kinkie
Copy link
Contributor Author

kinkie commented Mar 11, 2024

The particular & you have marked in bold is the query-string delimiter which should not be URL-encoded.

Yes; it's mostly a note to self, I haven't had the time to investigate where that url-encoding is taking place

src/errorpage.cc Outdated Show resolved Hide resolved
src/errorpage.cc Outdated
/* - TimeStamp */
str.appendf("TimeStamp: %s\r\n\r\n", Time::FormatRfc1123(squid_curtime));
body << "TimeStamp: " << Time::FormatRfc1123(squid_curtime) << "\r\n\r\n"
"ClientIP: " << src_addr << "\r\n";
Copy link
Contributor

Choose a reason for hiding this comment

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

This new/PR code is implicitly using Ip::Address::toUrl(). Official code uses Ip::Address::toStr(). The two methods produce different output. If this change is desirable, please disclose it (with its rationale and effect) in the PR description. Otherwise, refactor to avoid an undesirable change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for picking this up; reverted to the previous output

Copy link
Contributor

Choose a reason for hiding this comment

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

This change is still present. Only change was the line it occurs on - which confuses github into marking it "outdated".

src/errorpage.cc Outdated Show resolved Hide resolved
@kinkie kinkie requested a review from rousskov March 11, 2024 14:27
LegacyInputIterator operator "*" is supposed to return reference, not
value_type.
@kinkie kinkie requested a review from rousskov March 13, 2024 10:04
@rousskov rousskov dismissed their stale review March 13, 2024 21:29

Thank you for addressing my change requests.

@rousskov rousskov removed their request for review March 13, 2024 21:29
@rousskov rousskov added the S-waiting-for-author author action is expected (and usually required) label Mar 13, 2024
@kinkie kinkie removed the S-waiting-for-author author action is expected (and usually required) label Mar 14, 2024
@kinkie kinkie requested a review from rousskov March 14, 2024 06:16
rousskov

This comment was marked as outdated.

@kinkie kinkie added M-cleared-for-merge https://github.com/measurement-factory/anubis#pull-request-labels S-could-use-an-approval An approval may speed this PR merger (but is not required) labels Mar 14, 2024
@kinkie kinkie changed the title WIP: Refactor ErrorState::Dump to use PackableStream Refactor and improve ErrorState::Dump Mar 14, 2024
@rousskov rousskov removed the S-could-use-an-approval An approval may speed this PR merger (but is not required) label Mar 20, 2024
@yadij yadij removed the S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box label Apr 8, 2024
squid-anubis pushed a commit that referenced this pull request Apr 8, 2024
Rework the internals for generating output in ErrorState::Dump,
used for expanding the '%W' token in error page templates.

Also fix a bug with excessive html-quoting of the output.
@squid-anubis squid-anubis added the M-waiting-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels label Apr 8, 2024
@squid-anubis squid-anubis added M-merged https://github.com/measurement-factory/anubis#pull-request-labels and removed M-waiting-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels M-cleared-for-merge https://github.com/measurement-factory/anubis#pull-request-labels labels Apr 8, 2024
kinkie added a commit to kinkie/squid that referenced this pull request Apr 9, 2024
Rework the internals for generating output in ErrorState::Dump,
used for expanding the '%W' token in error page templates.

Also fix a bug with excessive html-quoting of the output.
kinkie added a commit to kinkie/squid that referenced this pull request Apr 9, 2024
Rework the internals for generating output in ErrorState::Dump,
used for expanding the '%W' token in error page templates.

Also fix a bug with excessive html-quoting of the output.
kinkie added a commit to kinkie/squid that referenced this pull request Apr 14, 2024
Rework the internals for generating output in ErrorState::Dump,
used for expanding the '%W' token in error page templates.

Also fix a bug with excessive html-quoting of the output.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
M-merged https://github.com/measurement-factory/anubis#pull-request-labels
Projects
None yet
4 participants