Skip to content

4.19.0 breaks passing new URL() into res.redirect(...) #5554

Closed

Description

@kavika-1

Understandably, res.redirect(...) should take a string. But for years we have been successfully passing a new URL(). We can certainly fix this on our side, but wanted to give a heads-up that this change might cause others to run into the same thing.

Error ultimately is thrown here:

reponse.js:915
var lowerLoc = loc.toLowerCase();

Activity

  1. wesleytodd commented on Mar 20, 2024

    @wesleytodd
    Member

    Ahhhh, yep that is not covered in the tests. I can prep a patch for that.

  2. wesleytodd commented on Mar 20, 2024

    @wesleytodd
    Member

    @kavika-1 Do you have a code snippet example of what you are doing so I can make sure this covers your case?

  3. added a commit that references this issue on Mar 20, 2024
    b06f533
  4. wesleytodd commented on Mar 20, 2024

    @wesleytodd
    Member

    Can you check the PR to ensure it would cover you? CI is failing because of #5552 but I can merge this all together for the patch if this will fix your report.

  5. kavika-1 commented on Mar 20, 2024

    @kavika-1
    Author

    Example Code snippet, which should be fixed with this patch, thanks!

        const url = new URL(config.url);
        if (req.query.app) {
          url.searchParams.append('app', req.query.app);
        }
        res.redirect(url);
    
  6. added 2 commits that reference this issue on Mar 20, 2024
    b70c174
    51584c7
  7. wesleytodd commented on Mar 20, 2024

    @wesleytodd
    Member

    Published with 4.19.1. Thanks for the quick report, this would certainly have been something lots of folks hit.

  8. kavika-1 commented on Mar 20, 2024

    @kavika-1
    Author

    Amazing turnaround, thanks @wesleytodd!

  9. ddolcimascolo commented on Mar 21, 2024

    @ddolcimascolo

    Ahah nice. I was hit by this and double checked the documentation. Clearly res.location accepts only a string so I patched my code to pass url.href instead of the url object and I could merge 4.19.0 just to discover a PR upgrading to 4.19.1 moments later for exactly this issue 😊

    Thanks for your reactivity!

    Cheers,
    David

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions