login_redirect_form_user_login_alter() processes the query parameter value with check_plain(filter_xss($params[$parameter_name])). The custom submission handler (login_redirect_user_login_submit()) then extracts the value and does the same thing again: check_plain(filter_xss($form_state['values']['destination']))
I can't see any advantage to any of that, and I can see problems with it.
Form API's #type='value' is used to store the destination URL, which means it doesn't appear in the form HTML at all, and therefore is surely not subject to any XSS issues? Basically it's stored in a PHP array, retrieved, and passed to drupal_goto().
filter_xss() and check_plain() might, on the other hand, corrupt an otherwise-valid URL.
I looks to me as if the destination should be processed with valid_url() and drupal_strip_dangerous_protocols(), and nothing more.
Am I missing something here?
| Comment | File | Size | Author |
|---|---|---|---|
| #4 | login_redirect-remove_extra_url_check-2639618-3.patch | 692 bytes | perennial.sky |
| #2 | login_redirect-remove_extra_url_check-2639618-1.patch | 592 bytes | perennial.sky |
Comments
Comment #2
perennial.sky commentedI agree, we already check that URL in form alter, checking URL in form submit doesn't make sense
Here is the patch
Comment #4
perennial.sky commentedHere is the new patch, I think we don't need to check URL is valid or not in form submit, we already do that in form alter.
Comment #6
perennial.sky commentedComment #7
jweowu commentedHi again. I appreciate that you wanted to attribute the change to me (especially when many maintainers do not attribute authorship when committing a patch), but it's really important that you only do that when the person actually wrote the code being committed!
In this case I did not write any code, and you've specified me as the author. This is the wrong thing to do -- and actually rather frustrating to me when the committed code has bugs: It looks like you've broken both of the functions that you changed, as each one now refers to a variable to which no value has been assigned.
(I now notice that you've followed that up with a commit to fix those bugs -- not under my name, which I can't decide whether is better or worse in the circumstances :)
Your intentions were certainly good, but please make sure that in future you only attribute authorship to someone who wrote the code.
(n.b. I've never tried rebasing a branch on d.o. so I'm not sure whether drupalcode.org allows people to force-push a revised history; but if that's possible I wouldn't object to you squashing those commits into a single authored-by-yourself replacement commit.)
Comment #8
perennial.sky commentedHello jweowu,
I am very thankful that you took your time and explained me in such a nice manner Actually, I saw the issue description and you explained the issue nicely that why I thought to give you some credit, so I made you the author of this commit, But I will next time be sure to attribute authorship to someone who wrote the code.