fix(vercel): clear artifacts from redirects #9287
Conversation
🦋 Changeset detectedLatest commit: 149121f The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
github-actions
bot
added
pkg: integration
|
Hm, I'm not sure if we support external intentionally, let me check. |
|
Yeah, it was an explicit non-goal of the RFC: https://github.com/withastro/roadmap/blob/main/proposals/0041-redirects.md#non-goals I wonder how many people are hitting this. I wouldn't mind revisiting this decision, but to do so in a more formal way, as redirects have a bit of complexity to them that I wouldn't want to just enable external in a one-off way. I'm surprised I didn't already do this, but would it make sense to check if the |
|
Yeah, an error seems the way to go. |
github-actions
bot
added
the
pkg: astro
| destination = to.destination | ||
| } | ||
| if (destination.startsWith("http")) { | ||
| return logger.error('redirects', `Redirecting to an external URLs is not supported: ${from} -> ${to}`); |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
|
The changes in |
|
Since we already have VT tests using external redirects, I am switching the "warn and ignore" to a "warn". |
There was a problem hiding this comment.
I'm happy with these changes! Would love @matthewp to confirm as well.
| else { | ||
| destination = to.destination | ||
| } | ||
| if (destination.startsWith("http")) { |
There was a problem hiding this comment.
Is 'httpfoobar' a valid pathname destination? Do we enforce that the pathname starts with /? I guess not since this gets here. So should we be checking for http(s)://?
There was a problem hiding this comment.
Makes sense, we don't validate the paths much.
| @@ -27,6 +28,15 @@ describe('Astro.redirect', () => { | |||
| expect(response.headers.get('location')).to.equal('/login'); | |||
| }); | |||
|
|
|||
| // ref: https://github.com/withastro/astro/pull/9287 | |||
| it.skip('Ignores external redirect', async () => { | |||
There was a problem hiding this comment.
can this be unskipped with this test?
There was a problem hiding this comment.
No, because of the vt test which tests for the opposite.
Changes
Testing
Adjusted pre-existing tests
Docs
Does not affect usage