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.

Sign up for GitHub

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

Jump to bottom

short url: ensure absolute path isn't persisted #6581

Merged
merged 5 commits into from
Mar 22, 2016
Merged

short url: ensure absolute path isn't persisted #6581

merged 5 commits into from
Mar 22, 2016

Conversation

jbudz
Copy link
Member

@jbudz jbudz commented Mar 18, 2016

The /shorten relative url was calculated by combing protocol, host, port, base url and removing the match from an absolute url. In cases when port was set to 443 or 80, the absolute url may not contain port causing the match to fail and resulting in absolute urls being persisted.

Instead of matching and removing, this parses the absolute url passed and constructs a relative url from the parts.

I changed shortenUrl to return an angular promise - I wasn't able to get past await when using $httpBackend to mock test responses.

Fixes #6127, although this is a different bug than the original issue.

spalger
spalger reviewed Mar 18, 2016
View reviewed changes
src/ui/public/share/__tests__/url_shortener.js
});
$httpBackend.flush();
});
});
Copy link
Contributor

Choose a reason for hiding this comment

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

would like to see a test with urls that have basepaths, query strings, no hash, etc.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@spalger
Copy link
Contributor

spalger commented Mar 18, 2016

Can we add some validation server side?

spalger
spalger reviewed Mar 18, 2016
View reviewed changes
src/ui/public/share/lib/url_shortener.js
throw err;
}
return $http.post(`${basePath}/shorten`, formData).then((result) => {
return `${parsedUrl.protocol}//${parsedUrl.host}${basePath}/goto/${result.data}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest using url.format()

@jbudz
Copy link
Member Author

jbudz commented Mar 21, 2016

jenkins, test it

1 similar comment
@jbudz
Copy link
Member Author

jbudz commented Mar 21, 2016

jenkins, test it

@spalger
Copy link
Contributor

spalger commented Mar 22, 2016

LGTM

@epixa epixa assigned epixa and unassigned BigFunger Mar 22, 2016
@epixa
Copy link
Contributor

epixa commented Mar 22, 2016

LGTM

epixa added a commit that referenced this pull request Mar 22, 2016
@epixa
Merge pull request #6581 from jbudz/issues/6127
d66df55 
short url: ensure absolute path isn't persisted
@epixa epixa merged commit d66df55 into elastic:master Mar 22, 2016
This was referenced Mar 22, 2016
Merged
Merged
Merged
Merged
epixa added a commit that referenced this pull request Mar 22, 2016
@epixa
Backport PR #6581 to 4.x
f8a872e 
[Commit 1]
short url: ensure absolute path isn't persisted

Original sha: a8c1305
Authored by Jonathan Budzenski <jbudz@users.noreply.github.com> on 2016-03-18T20:02:53Z
Committed by Jonathan Budzenski <jbudz@users.noreply.github.com> on 2016-03-21T15:57:16Z

[Commit 2]
[short url] use url.format when creating /goto link

Original sha: 2a71673
Authored by Jonathan Budzenski <jbudz@users.noreply.github.com> on 2016-03-21T14:09:04Z
Committed by Jonathan Budzenski <jbudz@users.noreply.github.com> on 2016-03-21T15:57:16Z

[Commit 3]
[short url] Add tests for query strings, no hashes

Original sha: 690a140
Authored by Jonathan Budzenski <jbudz@users.noreply.github.com> on 2016-03-21T14:42:10Z
Committed by Jonathan Budzenski <jbudz@users.noreply.github.com> on 2016-03-21T15:57:16Z

[Commit 4]
[short url] Cleanup

Original sha: 49d297f
Authored by Jonathan Budzenski <jbudz@users.noreply.github.com> on 2016-03-21T15:09:35Z
Committed by Jonathan Budzenski <jbudz@users.noreply.github.com> on 2016-03-21T15:57:16Z

[Commit 5]
[short url] Add server tests

Original sha: a612a0e
Authored by Jonathan Budzenski <jbudz@users.noreply.github.com> on 2016-03-21T15:30:12Z
Committed by Jonathan Budzenski <jbudz@users.noreply.github.com> on 2016-03-21T18:12:25Z
epixa added a commit that referenced this pull request Mar 22, 2016
@epixa
Backport PR #6581 to 4.5
4c0657a 
[Commit 1]
short url: ensure absolute path isn't persisted

Original sha: a8c1305
Authored by Jonathan Budzenski <jbudz@users.noreply.github.com> on 2016-03-18T20:02:53Z
Committed by Jonathan Budzenski <jbudz@users.noreply.github.com> on 2016-03-21T15:57:16Z

[Commit 2]
[short url] use url.format when creating /goto link

Original sha: 2a71673
Authored by Jonathan Budzenski <jbudz@users.noreply.github.com> on 2016-03-21T14:09:04Z
Committed by Jonathan Budzenski <jbudz@users.noreply.github.com> on 2016-03-21T15:57:16Z

[Commit 3]
[short url] Add tests for query strings, no hashes

Original sha: 690a140
Authored by Jonathan Budzenski <jbudz@users.noreply.github.com> on 2016-03-21T14:42:10Z
Committed by Jonathan Budzenski <jbudz@users.noreply.github.com> on 2016-03-21T15:57:16Z

[Commit 4]
[short url] Cleanup

Original sha: 49d297f
Authored by Jonathan Budzenski <jbudz@users.noreply.github.com> on 2016-03-21T15:09:35Z
Committed by Jonathan Budzenski <jbudz@users.noreply.github.com> on 2016-03-21T15:57:16Z

[Commit 5]
[short url] Add server tests

Original sha: a612a0e
Authored by Jonathan Budzenski <jbudz@users.noreply.github.com> on 2016-03-21T15:30:12Z
Committed by Jonathan Budzenski <jbudz@users.noreply.github.com> on 2016-03-21T18:12:25Z
epixa added a commit that referenced this pull request Mar 22, 2016
@epixa
Backport PR #6581 to 4.5.0
e84f1b6 
[Commit 1]
short url: ensure absolute path isn't persisted

Original sha: a8c1305
Authored by Jonathan Budzenski <jbudz@users.noreply.github.com> on 2016-03-18T20:02:53Z
Committed by Jonathan Budzenski <jbudz@users.noreply.github.com> on 2016-03-21T15:57:16Z

[Commit 2]
[short url] use url.format when creating /goto link

Original sha: 2a71673
Authored by Jonathan Budzenski <jbudz@users.noreply.github.com> on 2016-03-21T14:09:04Z
Committed by Jonathan Budzenski <jbudz@users.noreply.github.com> on 2016-03-21T15:57:16Z

[Commit 3]
[short url] Add tests for query strings, no hashes

Original sha: 690a140
Authored by Jonathan Budzenski <jbudz@users.noreply.github.com> on 2016-03-21T14:42:10Z
Committed by Jonathan Budzenski <jbudz@users.noreply.github.com> on 2016-03-21T15:57:16Z

[Commit 4]
[short url] Cleanup

Original sha: 49d297f
Authored by Jonathan Budzenski <jbudz@users.noreply.github.com> on 2016-03-21T15:09:35Z
Committed by Jonathan Budzenski <jbudz@users.noreply.github.com> on 2016-03-21T15:57:16Z

[Commit 5]
[short url] Add server tests

Original sha: a612a0e
Authored by Jonathan Budzenski <jbudz@users.noreply.github.com> on 2016-03-21T15:30:12Z
Committed by Jonathan Budzenski <jbudz@users.noreply.github.com> on 2016-03-21T18:12:25Z
epixa added a commit that referenced this pull request Mar 22, 2016
@epixa
Merge pull request #6596 from elastic/jasper/backport/6581-4.x-issues…
3ed5902 
…/6127

[backport] PR #6581 to 4.x
jbudz pushed a commit that referenced this pull request Mar 22, 2016
@epixa @jbudz
Backport PR #6581 to 4.4
5621e25 
[Commit 1]
short url: ensure absolute path isn't persisted

Original sha: a8c1305
Authored by Jonathan Budzenski <jbudz@users.noreply.github.com> on 2016-03-18T20:02:53Z
Committed by Jonathan Budzenski <jbudz@users.noreply.github.com> on 2016-03-21T15:57:16Z

[Commit 2]
[short url] use url.format when creating /goto link

Original sha: 2a71673
Authored by Jonathan Budzenski <jbudz@users.noreply.github.com> on 2016-03-21T14:09:04Z
Committed by Jonathan Budzenski <jbudz@users.noreply.github.com> on 2016-03-21T15:57:16Z

[Commit 3]
[short url] Add tests for query strings, no hashes

Original sha: 690a140
Authored by Jonathan Budzenski <jbudz@users.noreply.github.com> on 2016-03-21T14:42:10Z
Committed by Jonathan Budzenski <jbudz@users.noreply.github.com> on 2016-03-21T15:57:16Z

[Commit 4]
[short url] Cleanup

Original sha: 49d297f
Authored by Jonathan Budzenski <jbudz@users.noreply.github.com> on 2016-03-21T15:09:35Z
Committed by Jonathan Budzenski <jbudz@users.noreply.github.com> on 2016-03-21T15:57:16Z

[Commit 5]
[short url] Add server tests

Original sha: a612a0e
Authored by Jonathan Budzenski <jbudz@users.noreply.github.com> on 2016-03-21T15:30:12Z
Committed by Jonathan Budzenski <jbudz@users.noreply.github.com> on 2016-03-21T18:12:25Z
epixa added a commit that referenced this pull request Mar 22, 2016
@epixa
Merge pull request #6599 from elastic/jasper/backport/6581-4.5-issues…
4e261e2 
…/6127

[backport] PR #6581 to 4.5
epixa added a commit that referenced this pull request Mar 22, 2016
@epixa
Merge pull request #6597 from elastic/jasper/backport/6581-4.5.0-issu…
13229cf 
…es/6127

[backport] PR #6581 to 4.5.0
epixa added a commit that referenced this pull request Mar 22, 2016
@epixa
Merge pull request #6598 from elastic/jasper/backport/6581-4.4-issues…
5d4bec9 
…/6127

[backport] PR #6581 to 4.4
@spalger spalger mentioned this pull request Mar 31, 2016
Closed
@epixa epixa added v4.5.2 and removed v4.5.1 labels May 14, 2016
@epixa epixa removed the v4.4.3 label Aug 4, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@epixa epixa

Labels
review v4.5.0 v4.5.2 v4.6.0 v5.0.0-alpha1 v5.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jbudz @spalger @epixa @BigFunger