Skip to content

feat(res.redirect): add validation for url and status arguments - #6404

Open
bjohansebas wants to merge 2 commits into
masterfrom
throw-redirect
Open

feat(res.redirect): add validation for url and status arguments#6404
bjohansebas wants to merge 2 commits into
masterfrom
throw-redirect

Conversation

@bjohansebas

Copy link
Copy Markdown
Member

Validations are being added, this would ideally be for Express 6. Maybe sending a deprecation message would be great, what do you think?

closes#6391

@UlisesGasconUlisesGascon added the semver-major This change is a semver major label Mar 18, 2025

@UlisesGasconUlisesGascon left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think is a good idea, but we will need to introduce the deprecation notice in 5.x before merging this PR 🤔

Comment threadtest/res.redirect.js
varapp=express();

app.use(function(req,res){
res.redirect("300",'http://google.com')

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
res.redirect("300",'http://google.com')
res.redirect("300",'https://google.com')

http -> https

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not bad, but also just to be clear it is not necessary in the tests.

Comment threadlib/response.js
Comment on lines 836 to 839
if (typeof status !== 'number') {
deprecate('Status must be a number');
throw new TypeError('res.redirect: status must be a number');
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Status is set using this.status(status), which performs type validation (#4212). Earlier status is used to get a status message for body, but passing something invalid there is not a problem if an error is thrown before it's sent.

Suggested change
if(typeofstatus!=='number'){
deprecate('Status must be a number');
thrownewTypeError('res.redirect: status must be a number');
}

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

6.xenhancementsemver-majorThis change is a semver major

Projects

None yet

Development

Successfully merging this pull request may close these issues.

res.redirect(undefined) sends invalid Location: undefined header Throw when the path is undefined in res.redirect.

5 participants

@bjohansebas@wesleytodd@UlisesGascon@krzysdz@shivarm