adding `_scheme` parameter to `url_for` #667

Merged
merged 1 commit into from Jan 25, 2013

Projects

None yet

4 participants

@maxcountryman

In order to better facilitate generation of URLs that make use of an HTTPS URL
scheme this patch adds a parameter with this specific purpose in mind. To
achieve this we explicitly pass in a param, _scheme='https', and then set the
url_scheme attribute of our MapAdapter instance appropriately.

Importantly, _external=True must be set in order for this to work properly.
As such, failure to do so results in a ValueError being raised.

@xsleonard

I'm generating urls in some of my email message templates (confirmation, password reset), and I would like them to be secure urls, but the source from where they are generated is not necessarily https.

Current I am doing this to handle the problem,
https://gist.github.com/4561140

But it would be nice to not need this workaround

@untitaker
The Pallets Projects member

I think the new keyword argument of url_for is a good idea, but I don't see the point of secure_url_for. Also I think the _secure arg should imply _external. Explicitly setting _external to False and _secure to True would result in a ValueError.

@maxcountryman

I'm happy to omit secure_url_for if the _secure flag is deemed sufficient. It's trivial to enforce _external btw, which incidentally secure_url_for does do. However, a ValueError is seemingly the appropriate response where a user explicitly passes in _secure=True and _external=False: this is in fact an error and should raise an error. It would be sneaky and a little confusing to override an explicit pass in of _external=False imo.

@untitaker
The Pallets Projects member

Could you add the handing for url_for(..., _secure=True, _external=False) please?

@maxcountryman

@untitaker do you mean raise an explicit ValueError, e.g. ValueError('_external must be True where _secure is True')? Otherwise I do think raising a ValueError makes sense, but it might be best to do it explicitly in the scope of url_for.

@untitaker
The Pallets Projects member
@untitaker
The Pallets Projects member

What does the overlord of elegant APIs @kennethreitz have to say about this?

@maxcountryman

I don't know that it hurts to provide a helper function, after all, that's what helpers are there for. But as I said before, I'll defer to the maintainers. I'd just like to get this in as there seems to be a need for it. Out of curiosity, what relationship do you have to the Flask project, @untitaker?

@kennethreitz

Why not just url_for(secure=True) ?

@untitaker
The Pallets Projects member

About as much as you have @maxcountryman

@untitaker
The Pallets Projects member

@kennethreitz The fear is that this could interfere with the user's keyword arguments for the view.

@maxcountryman

@kennethreitz I think url_for(..., _secure=True) is more consistent with the API fwiw.

@kennethreitz

that would be a terrible kwarg :)

@maxcountryman

@kennethreitz so is _external=True, but that's what we have. :)

@maxcountryman

I think at this point it's probably worse to be inconsistent with the existing API than to try to retroactively correct it. So for that reason I preferred _secure.

I still need feedback on whether or not to include secure_url_for as a helper from @kennethreitz or @mitsuhiko.

Finally I can include an explicit ValueError within the scope of url_for if it's not sufficient to leave this lower levels of the call stack.

How does that sound?

@untitaker
The Pallets Projects member

Another aspect: I find "_secure" as an arg too ambiguous, since it also could mean that the url will be escaped to prevent XSS or something like that. "_https" seems like a better name (and also https_url_for)

@untitaker
The Pallets Projects member
@kennethreitz

(_scheme=)

@kennethreitz

The more I think about this, the more I feel it is unnecessary, I suppose. If you want something to be SSL, why wouldn't everything already be SSL?

@maxcountryman

@kennethreitz read @xsleonard's comment for real world use-case.

@kennethreitz

hmm, I guess I like url_for(_scheme='https') then

@maxcountryman

@kennethreitz how about a helper? Yea or nay? (I don't have a particularly strong opinion, but it does seem like a clear shortcut and it clearly exposes the functionality.)

@untitaker
The Pallets Projects member

Agreed Kennneth, a scheme arg might be even better.

@maxcountryman

I've force-pushed some updates to this guy. I think this should be more in line with your suggestions @kennethreitz.

@maxcountryman maxcountryman adding `_scheme` parameter to `url_for`
In order to better facilitate generation of URLs that make use of an HTTPS URL
scheme this patch adds a parameter with this specific purpose in mind. To
achieve this we explicitly pass in a param, `_scheme='https'`, and then set the
`url_scheme` attribute of our `MapAdapter` instance appropriately.

Importantly, `_external=True` must be set in order for this to work properly.
As such, failure to do so results in a `ValueError` being raised.
b5069d0
@kennethreitz

Seems harmless enough.

@kennethreitz kennethreitz merged commit b975dd4 into pallets:master Jan 25, 2013

1 check failed

Details default The Travis build failed
@maxcountryman maxcountryman deleted the maxcountryman:secure-url-for branch Jan 25, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment