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

[5.3] Router: Discover tainted URLs for core components #44477

Merged
merged 2 commits into from
Dec 3, 2024

Conversation

Hackwar
Copy link
Member

@Hackwar Hackwar commented Nov 18, 2024

Summary of Changes

The routing in Joomla has had 2 issues in the past: When IDs were enabled in URLs you could basically modify the alias part of the URL at will and if you wanted to switch from ID based URLs to those without, there was no way to automatically redirect everything. This PR tries to fix both issues.

The PR is depending on #44455 to mark a parsed URL as tainted. There are two modes here:

This PR was made possible by the support of djumla. Thank you for that.

IDs switched on

When reading the content item or category segment of a URL, the ID is read and based on that ID the alias is read from the database and if the two don't match, the URL is marked as tainted and later redirected to the "correct" URL. (#43992 is fixing the alias for that URL then) This prevents modified aliases in the URL.

IDs switched off

If IDs are switched off, we are first searching for the normal content item or category as we are doing now, but when we don't find a match, we assume that we are actually reading a legacy URL with IDs switched on. In that case we mark the URL as tainted (regardless of it being totally broken or not, since when we fail to parse the URL at all, we already throw a 404 before we reach the handling of tainted URLs) and then compare the segment with the segment we would get with IDs enabled. If that matches, we return the ID and later redirect to the correct URL. This means that you can switch from ID-based URLs to "clean" URLs and Joomla will automatically redirect all pages to the correct URLs.

Testing Instructions

  1. Apply both this PR and [5.3] Routing: Allow to mark parsed URLs as tainted #44455 and switch on IDs in the integration tab of either com_content, com_contact or com_newsfeeds. Surf to URLs in your frontend containing IDs and modify the alias of either the category or the content item (article, contact or newsfeed) and load that URL.
  2. Disable IDs for the component and reload the ID-based URL

Actual result BEFORE applying this Pull Request

for 1.: You get the same content with more than one URL
for 2.: You get a 404 error

Expected result AFTER applying this Pull Request

for 1.: You are redirected to the right URL again with the correct alias.
for 2.: You are redirected to the right URL without IDs

Link to documentations

Please select:

  • Documentation link for docs.joomla.org:

  • No documentation changes for docs.joomla.org needed

  • Pull Request link for manual.joomla.org:

  • No documentation changes for manual.joomla.org needed

@brianteeman
Copy link
Contributor

I have tested this item ✅ successfully on 1e5d91a


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/44477.

@SniperSister
Copy link
Contributor

I have tested this item ✅ successfully on 1e5d91a

Works as described


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/44477.

@Hackwar
Copy link
Member Author

Hackwar commented Nov 21, 2024

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/44477.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Nov 21, 2024
@rdeutz rdeutz merged commit 2e99a5b into joomla:5.3-dev Dec 3, 2024
4 checks passed
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Dec 3, 2024
@rdeutz
Copy link
Contributor

rdeutz commented Dec 3, 2024

Thanks

@rdeutz rdeutz added this to the Joomla! 5.3.0 milestone Dec 3, 2024
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.

6 participants