Skip to content

Conversation

@Lukewh
Copy link
Contributor

@Lukewh Lukewh commented Mar 17, 2023

Done

By default, redirects are very restrictive, this change makes things more open by default, which is fine in 99% of our use-cases.

QA

  • You'll need to include this branch in your project.
  • Add a couple of redirects to your discourse docs:
    1. /docs/test -> /docs
    2. /docs/test -> https://github.com
  • With your Docs initialised with url_prefix="/docs":
    • limit_redirects_to_url_prefix set to True redirect 1. should work and redirect 2. should not.
    • limit_redirects_to_url_prefix unset (or set to False), both redirects should work

Fixes https://warthogs.atlassian.net/browse/WD-2871

@sparkiegeek
Copy link
Contributor

conflicts with main now

"</tr>"
"<tr>"
"<td>/docs/other/path</td>"
"<td>/scod/cooler-place</td>"
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't you want to be including this target in an assert?

api=discourse_api,
index_topic_id=1,
url_prefix="/",
limit_redirects_to_url_prefix=False,
Copy link
Contributor

Choose a reason for hiding this comment

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

I have a few wonderings:

  • Should a limit on redirect targets perhaps be the default behaviour for all documentation?
  • Or, would this be better as a redirect_prefix setting, so you can set the allowed prefix?
  • Or should we combine the two, with the default being restrictive but overrideable with a different prefix?

Really the only way to answer this is to do a quick audit of what sorts of redirects currently exist within documentation sets. Do people actually redirect out of the domain, or out of the docs area, in practice?

Copy link
Contributor

@anthonydillon anthonydillon left a comment

Choose a reason for hiding this comment

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

@Lukewh can you rebase this one please?

warnings.append(
(
f"Redirect map location {location} "
f"is blocked by same prefix constraint"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it same prefix or same origin?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same prefix, e.g. "/docs"

@Lukewh
Copy link
Contributor Author

Lukewh commented Aug 23, 2023

I'll fix the tests later - looks like I need to update new ones

@anthonydillon
Copy link
Contributor

@Lukewh is this still valid?

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.

5 participants