Skip to content

Fix url_rewriter.hosts not used for output_add_rewrite_var() #13294

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

Conversation

haszi
Copy link
Contributor

@haszi haszi commented Jan 31, 2024

Fixes the bug where output_add_rewrite_var() is using the hosts set in session.trans_sid_hosts instead of the url_rewriter.hosts which was added specifically for this purpose.

@haszi
Copy link
Contributor Author

haszi commented Jan 31, 2024

MACOS failure is unrelated (libcurl error messages changed).

@haszi haszi force-pushed the Fix-session.trans_sid_hosts-used-instead-of-url_rewriter.hosts branch from 1f56c6f to 3cd1a02 Compare February 2, 2024 10:01
@haszi
Copy link
Contributor Author

haszi commented Feb 2, 2024

After rebasing to trigger checks again, MACOS fails three Zend tests unrelated to this PR.

@haszi
Copy link
Contributor Author

haszi commented Feb 22, 2024

I know this is not a priority as this functionality never worked correctly, but could I ask for a review?

@bukka
Copy link
Member

bukka commented Feb 25, 2024

I will try to find some time on this in the next few weeks. Ping me in case there is no review in the next 3 weeks.

@haszi
Copy link
Contributor Author

haszi commented Mar 19, 2024

I will try to find some time on this in the next few weeks. Ping me in case there is no review in the next 3 weeks.

@bukka pinging you as it has been a little over 3 weeks.

@bukka bukka closed this in 6150bf5 Mar 22, 2024
@bukka
Copy link
Member

bukka commented Mar 22, 2024

I have done a bit of testing and it all looked good to me. I also noticed that it was incorrectly using session for url-rewriter:

072- <form action="//url-rewriter.com/bar.php" method="get"></form>
073- <form action="http://url-rewriter.com/bar.php" method="get"></form>
072+ <form action="//url-rewriter.com/bar.php" method="get"><input type="hidden" name="PHPSESSID" value="testid" /></form>
073+ <form action="http://url-rewriter.com/bar.php" method="get"><input type="hidden" name="PHPSESSID" value="testid" /></form>

(this was previous master run without the patch applied)

So it was actually other way round in some cases, right? I'm thinking that I should maybe extend upgrading note (maybe check if the current info makes sense to you)

Due to potentially BC break I picked only master for this which this PR also targeted.

@haszi
Copy link
Contributor Author

haszi commented Mar 22, 2024

I have done a bit of testing and it all looked good to me. I also noticed that it was incorrectly using session for url-rewriter:

So it was actually other way round in some cases, right? I'm thinking that I should maybe extend upgrading note (maybe check if the current info makes sense to you)

Yes, the end result of the rewriting was the other way around in some cases (not the mix-up of host lists though). I don't remember the exact details but I think this had something to do with the code below where php_url_scanner_ex_activate(type); wiped out the type of the rewriter that then defaulted to 0. As this is the URL rewriter's type the session ID was added to the URL rewriter's variable list.

if (!url_state->active) {
php_url_scanner_ex_activate(type);
php_output_start_internal(ZEND_STRL("URL-Rewriter"), handler, 0, PHP_OUTPUT_HANDLER_STDFLAGS);
url_state->active = 1;
url_state->type = type;
}

@haszi haszi deleted the Fix-session.trans_sid_hosts-used-instead-of-url_rewriter.hosts branch March 22, 2024 15:48
Girgias pushed a commit to php/doc-en that referenced this pull request Jan 27, 2025
)

Document the bugfix of php/php-src/pull/13294. This is a part of #3872.

Co-authored-by: haszi <haszika80@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants