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

fix(AjaxObservable): Use encodeURIComponent to encode request body #2399

Closed
wants to merge 97 commits into from

Conversation

ericponto
Copy link

@ericponto ericponto commented Feb 21, 2017

Description:
The contentType 'application/x-www-form-urlencoded' was using encodeURI but should be using
encodeURIComponent on the request body.

Related issue (if exists):
closes #2389

The contentType 'application/x-www-form-urlencoded' was using `encodeURI` but should be using
`encodeURIComponent` on the request body.

closes ReactiveX#2389
@coveralls
Copy link

Coverage Status

Coverage remained the same at 97.688% when pulling 011ad8a on ericponto:fix-ajax-encoding into 023d436 on ReactiveX:master.

@benlesh
Copy link
Member

benlesh commented Feb 21, 2017

The contentType 'application/x-www-form-urlencoded' was using encodeURI but should be using
encodeURIComponent on the request body.

@ericponto interesting. Can you please provide a link or additional information on why this change is required?

@ericponto
Copy link
Author

Sure! I looked at MDN before making the PR and it has pretty good info on the difference.

encodeURI

Assumes that the URI is a complete URI, so does not encode reserved characters that have special meaning in the URI.

encodeURIComponent

To avoid unexpected requests to the server, you should call encodeURIComponent on any user-entered parameters that will be passed as part of a URI.

When encoding the request body params it is the latter use case since it is not a complete URI.

Additionally, I took at look at how jQuery handles it (since probably more ajax request have used jQuery than anything else), and it uses $.param on ajax data which uses encodeURIComponent.

Document 'using' operator, describing what
it accepts and returns. Describe how resulting
Observable will behave and when unsubscribe
method from resource object will be called.
@gernsdorfer
Copy link

This PR is required because:
If a value of an Object contains a character like &, encode ignore this character.

Here i've the following problem:

BodyParameters: { name1: "value1&test" }
Sended Parameters via http(post):
name1=value1
test=

mpodlasin and others added 23 commits April 2, 2017 19:43
Describe what parameters 'finally' accepts. Emphasize
when function passed to the operator is called. Compare the
operator with 'subscribe'. Add examples for calling function
passed to 'finally' on complete and on unsubscribe.
…tiveX#2497)

BREAKING CHANGE: IteratorObservable no longer share iterator between
subscription

- closes ReactiveX#2496
* test(partition): remove nonsense test

Test wasn't testing anything, it was passing the partition function
itself to the expect(), which doesn't really do anything of value.

* test(repeat): remove nonsense tests

These tests were really testing the rethrowing capability of Observable, as triggered when the scheduler was flushed. The call directly above the flush to `repeat` does nothing but create a new observable that is never subscribed to
* chore(package): bump up typescript to latest

* chore(typings): remove deprecated typings

* chore(spec): remove redundant reference

* fix(error): custom error inherits via setPrototypeof

BREAKING CHANGE: IE10 and lower will need to polyfill `Object.setPrototypeOf`
- Removes Map polyfill
- Removes FastMap impl - Tests in latest Chrome and Node show no discernable difference in performance between Map<string, T> and Object has a hashtable for our use case.
- Adds an error that is thrown immediately at runtime if Map does not exist.

BREAKING CHANGE: Older runtimes will require Map to be polyfilled to use
`groupBy`
Gets rid of fancy polyfill and uses Promise to schedule instead, since Promise is required for several key parts of the library to work. Also setImmediate does not appear to be getting standardized.

BREAKING CHANGE: Old runtimes must polyfill Promise in order to use ASAP scheduling.
BREAKING CHANGE: Using `distinct` requires a `Set` implementation and must be polyfilled in older runtimes
benlesh and others added 17 commits January 11, 2018 20:30
BREAKING CHANGE: Symbols are no longer exported directly from modules such as `rxjs/symbol/observable` please use `Symbol.observable` and `Symbol.iterator` (polyfills may be required)
BREAKING CHANGE: Can no longer explicitly import types from `rxjs/interfaces`, import them from `rxjs` instead
BREAKING CHANGE: THIS NEGATES THE PREVIOUS BREAKING CHANGES TO OPERATOR MOVE TO `rxjs`, UPDATE CHANGELOG ON RELEASE
BREAKING CHANGE: All create functions such as `of`, `from`, `combineLatest` and `fromEvent` should now be imported from `rxjs/create`.
BREAKING CHANGE: `HotObservable` and `ColdObservable`, and other testing support types are no longer exported directly.
BREAKING CHANGE: Many internal use utilities like `isArray` are now hidden under `rxjs/internal`, they are implementation details and should not be used.
BREAKING CHANGE: Ajax observable should be imported from `rxjs/ajax`.
…ebsocket`

BREAKING CHANGE: `webSocket` creator function now exported from `rxjs/websocket` as `websocket`.
@rxjs-bot
Copy link

Messages
📖

CJS: 1381.5KB, global: 750.2KB (gzipped: 120.6KB), min: 145.4KB (gzipped: 31.4KB)

Generated by 🚫 dangerJS

@coveralls
Copy link

coveralls commented Jan 12, 2018

Coverage Status

Coverage remained the same at 97.688% when pulling 011ad8a on ericponto:fix-ajax-encoding into 023d436 on ReactiveX:master.

@winghouchan
Copy link

winghouchan commented Feb 1, 2018

This didn't make it into 5.5.6. Is there anything blocking this? If there are, is there anything that can be done? If there aren't, will it make it to the next release and will that be soon?

@benlesh benlesh changed the base branch from master to stable February 1, 2018 18:41
@benlesh
Copy link
Member

benlesh commented Mar 30, 2018

Something is really messed up with this PR. I'm going to close it and we can start another PR.

@ericponto
Copy link
Author

@benlesh I'll submit a new PR here in a bit.

@sfishel18
Copy link

sfishel18 commented Apr 24, 2018

I'm still seeing this issue in 5.5.10. Is there a way to know if/when the fix will end up in a 5.5.x release? Thanks much!

@lock
Copy link

lock bot commented Jun 5, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Jun 5, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.