Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

safeNextPathFrom(): update test cases #1007

Merged
merged 3 commits into from
Nov 6, 2023

Conversation

alxndrsn
Copy link
Contributor

No description provided.

@alxndrsn alxndrsn force-pushed the safe-next-test-tag-injection-1 branch from bc02fdd to 97fddf3 Compare September 27, 2023 14:40
@alxndrsn alxndrsn changed the title safeNextPathFrom(): add tests for bad content wip: safeNextPathFrom(): add tests for bad content Sep 27, 2023
@matthew-white
Copy link
Member

@alxndrsn, now that #1014 is merged, how are you thinking of this PR? I see new cases here that seem like they improve coverage (e.g., /login), as well as others that don't seem needed now that #1014 is merged (e.g., /login/foo/..).

@alxndrsn
Copy link
Contributor Author

@matthew-white the /login-related tests actually indicate a proposed change of behaviour: should safeNextPathFrom() redirect next=/login to the web root?

@matthew-white
Copy link
Member

the /login-related tests actually indicate a proposed change of behaviour

I don't quite see the change. The test has:

[ '/login',                          '/#/' ],

But isn't that what Backend already does? Or am I misunderstanding?

if (url.origin !== envDomain || url.pathname === '/login')
return '/#/';

should safeNextPathFrom() redirect next=/login to the web root?

No strong feelings! Unlike Frontend, Backend doesn't need special logic around next=/login. There's no problem if Backend redirects to /#/login. However, I can also see the case for keeping the logic around next the same in Frontend and Backend. Frontend redirects to /#/ for next=/login, and we could have Backend do the same. I think we should implement and test whichever way you think is best!

@alxndrsn alxndrsn force-pushed the safe-next-test-tag-injection-1 branch from 97fddf3 to 8c0ca0c Compare November 3, 2023 13:46
@alxndrsn
Copy link
Contributor Author

alxndrsn commented Nov 3, 2023

@matthew-white I've updated the PR to remove irrelevant tests, and add one for path traversal.

@alxndrsn alxndrsn changed the title wip: safeNextPathFrom(): add tests for bad content safeNextPathFrom(): update test cases Nov 3, 2023
@alxndrsn alxndrsn marked this pull request as ready for review November 3, 2023 13:55
@alxndrsn alxndrsn merged commit 22e7566 into getodk:master Nov 6, 2023
1 check passed
@alxndrsn alxndrsn deleted the safe-next-test-tag-injection-1 branch November 6, 2023 07:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants