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

Return to the edit mapping page after save #79

Merged
merged 2 commits into from Nov 21, 2013
Merged

Conversation

@fofr
Copy link
Contributor

@fofr fofr commented Nov 20, 2013

  • Redirect back to mapping edit screen so user can quickly confirm
    their changes or fix mistakes
  • Update messaging on create to give feedback about type or mapping,
    the canonicalised path and the redirect location if appropriate
Paul Hayes and others added 2 commits Nov 20, 2013
* Update messaging on create to give feedback about type or mapping,
the canonicalised path and the redirect location if appropriate
* Redirect back to mapping edit screen so user can quickly confirm
their changes or fix mistakes
@rgarner

This comment has been minimized.

Copy link
Contributor

@rgarner rgarner commented on app/controllers/mappings_controller.rb in 0106e19 Nov 21, 2013

I wouldn't normally expect to see a call to ActionController::Base.helpers.link_to in here - that would be more of a view concern. However, flash[:notice] overlaps a bit because you're having to set it in the controller. I think you can avoid the ugliness of the ActionController::Base reference by using view_context.link_to instead. If you're going to do that, I think you can move the whole if @mapping.redirect? block to MappingsHelper#created_notice, and just say redirect_to edit_site_mapping_path(@site, @mapping), notice: view_context.created_notice(@mapping)

@rgarner
Copy link
Contributor

@rgarner rgarner commented Nov 21, 2013

I tried it out here (hadn't used view_context before).

@jamiecobbett
Copy link
Contributor

@jamiecobbett jamiecobbett commented Nov 21, 2013

I agree with @rgarner, unless you have any objections @fofr?

@fofr
Copy link
Contributor Author

@fofr fofr commented Nov 21, 2013

👍 to @rgarner's suggestion

rgarner added a commit that referenced this pull request Nov 21, 2013
Return to the edit mapping page after save
@rgarner rgarner merged commit 7d868e3 into master Nov 21, 2013
@rgarner rgarner deleted the redirect-to-edit-on-save branch Nov 21, 2013
redirect_to site_mappings_path(@site), notice: 'Mapping saved.'
if @mapping.redirect?
link = ActionController::Base.helpers.link_to @mapping.new_url, @mapping.new_url
notice = "Mapping created. <strong>#{@mapping.path}</strong> redirects to <strong>#{link}</strong>"

This comment has been minimized.

@jamiecobbett

jamiecobbett Nov 21, 2013
Contributor

I'm concerned that this is potentially an XSS vector because @mapping.path and @mapping.new_url both come from user-input.

I was able to insert a script tag into the flash message output by providing a malicious path: /foo/><script>alert(undefined);</script>

ERB provides a html_escape method, aliased to h() that escapes HTML, which we can use here. See:
http://api.rubyonrails.org/v3.2.11/classes/ERB/Util.html#method-c-html_escape

I believe (looks at @rgarner) that Rails automatically escapes input to the link_to helper so us doing so would be redundant. I strongly believe we should ERB::Util.html_escape the @mapping.path.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.