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

Properly encode slashes in path parameters #280

Closed
wants to merge 1 commit into from

Conversation

gwicke
Copy link

@gwicke gwicke commented Mar 9, 2015

Swagger spec version 2.0 only supports level 1 URI templating as described in
http://tools.ietf.org/html/rfc6570#section-3.2.2. This means that the entire
path component (including slashes) needs to be encoded with
encodeURIComponent.

Swagger spec version 2.0 only supports level 1 URI templating as described in
http://tools.ietf.org/html/rfc6570#section-3.2.2. This means that the entire
path component (including slashes) needs to be encoded with
encodeURIComponent.
Operation.prototype.encodePathParam = function(pathParam) {
var encParts, part, parts, i, len;
Copy link
Contributor

Choose a reason for hiding this comment

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

Replying this.

return pathParam.toString().split('/').map(encodeURIComponent).join('/')

The above code does exactly the same thing

@fehguy should we merge this change?

Copy link
Author

Choose a reason for hiding this comment

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

It does the same thing as the old code, but not the same thing as

encodeURIComponent(pathParam);

Copy link
Contributor

Choose a reason for hiding this comment

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

That's true.

encodeURIComponent('a/b') === "a%2Fb"
'a/b'.split('/').map(encodeURIComponent).join('/') === 'a/b'

if we decided to have an option to allow slash(/), we would need to skip encoding it. Your code skip encoding slash. My suggestion is to make that code shorter and less complex.

Based on @fehguy's comment we need to decide if we want to skip slash in encoding and if the answer to that is yes, we should make this behavior configurable.

Copy link
Author

Choose a reason for hiding this comment

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

@mohsen1, according to the URI template spec:

  • slashes need to be encoded for {foo} (the only variant supported in Swagger 2.0), and
  • slashes don't need to be encoded for {+foo}.

So both can be supported easily and within the same URL once the spec gains support for level 2 URI templating.

gwicke added a commit to gwicke/swagger-ui that referenced this pull request Mar 9, 2015
@fehguy
Copy link
Contributor

fehguy commented May 7, 2015

I believe the spec does not support nor prohibit encoding slashes in path params. Let me get input from @webron on this and if it is supported, I'll clean and merge this.

@webron
Copy link
Contributor

webron commented May 9, 2015

We don't support non-encoded slashes in path parameter values. However, I don't see a problem if they are indeed encoded.

Please note that this PR as against master and should not be merged.

fehguy added a commit that referenced this pull request Jun 3, 2015
@fehguy fehguy closed this Jun 3, 2015
@yonderblue
Copy link

The UI still seems to have this not encoding, this would seem like a pretty silly thing to not encode values the user enters in the boxes for path parameters. Is it planned to change this?

@webron
Copy link
Contributor

webron commented Jun 4, 2015

Which branch have you tested, @Gaillard?

@yonderblue
Copy link

Hey @webron, I tried the develop_2.0 branch for swagger-ui with the same effect.

Totally unrelated but also on the develop_2.0 branch when url into the SwaggerUi options is null it explodes, for those that are passing in a spec directly. Putting something silly in url fixes it it, I think it has to do with line 20949 of swagger-ui.js.

@webron
Copy link
Contributor

webron commented Jun 4, 2015

@Gaillard - please open two separate issues on the above then, we won't be able to keep track of those as comments on a closed PR.

@yonderblue
Copy link

will do thanks

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.

5 participants