Skip to content

Allow hash path in redirectUrl - #3035

Closed
ambrt wants to merge 1 commit into
strongloop:masterfrom
ambrt:master
Closed

Allow hash path in redirectUrl#3035
ambrt wants to merge 1 commit into
strongloop:masterfrom
ambrt:master

Conversation

@ambrt

@ambrtambrt commented Dec 16, 2016

Copy link
Copy Markdown
Contributor

Description

Allows GET variables and hash paths for redirecting user after email confirm.

Related issues

@slnode

Copy link
Copy Markdown

Can one of the admins verify this patch?

3 similar comments
@slnode

Copy link
Copy Markdown

Can one of the admins verify this patch?

@slnode

Copy link
Copy Markdown

Can one of the admins verify this patch?

@slnode

Copy link
Copy Markdown

Can one of the admins verify this patch?

@bajtosbajtos changed the title fixes #2307Allow hash path in redirectUrlDec 21, 2016

@bajtosbajtos 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.

Thank you for the updated patch, @ambrt.

I need you to add a unit-test covering this change - the test should fail with the current implementation and pass with your fix in place.

Comment threadcommon/models/user.js
return next(new Error(g.f('The transport does not support HTTP redirects.')));
}
ctx.res.location(ctx.args.redirect);
ctx.res.location(decodeURIComponent(ctx.args.redirect));

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 believe ctx.args.redirect should have been already url-decoded by strong-remoting, therefore this change is not necessary. Unless I am missing something?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

If its decoded then this change is not necessary.

@bajtosbajtos self-assigned this Dec 21, 2016
@bajtosbajtos added the bug label Jan 26, 2017
@bajtosbajtos mentioned this pull request Feb 6, 2017
2 tasks
@bajtos

Copy link
Copy Markdown
Member

Closing in favour of #3185.

@bajtosbajtos closed this Feb 6, 2017
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@ambrt@slnode@bajtos@cgole@crandmck