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

Honor url whitelist when fetching remote grass files #600

Merged
merged 3 commits into from
Jun 29, 2017

Conversation

pe4cey
Copy link
Contributor

@pe4cey pe4cey commented Jun 22, 2017

The url param when loading remote grass using :style needs to be present in the whitelist returned by the neo4j server

Copy link
Member

@oskarhane oskarhane left a comment

Choose a reason for hiding this comment

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

It rejects and refer to the host not being in the whitelist even though it is.
Try: https://oskarhane-dropshare-eu.s3-eu-central-1.amazonaws.com/grass-crCUgFn9vt/grass.json

I think I found the cause and made a comment on that line.

})

describe('Grass remote fetch', () => {
test('should not fetch from url not in the whitelist', () => {
Copy link
Member

@oskarhane oskarhane Jun 26, 2017

Choose a reason for hiding this comment

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

This test won't work if the function doesn't reject. Try changing urlNotInWhitelist = 'foo'.

Since Jest 20 you can return promises and expect it to reject. There's one missing piece though, you cannot match on error message. The workaround is to match an object and looks like this:

return expect(fetchRemoteGrass(urlNotInWhitelist, whitelist)).rejects.toMatchObject({
  message: 'Hostname is not allowed according to server whitelist'
})

https://facebook.github.io/jest/docs/expect.html#resolves and jestjs/jest#3601

const urlInWhitelist = 'foo'
return fetchRemoteGrass(urlInWhitelist, whitelist)
.then((res) => {
expect(res).toBe(urlInWhitelist)
Copy link
Member

Choose a reason for hiding this comment

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

Same as above, but using .resolves: https://facebook.github.io/jest/docs/expect.html#resolves

@@ -207,7 +207,7 @@ const availableCommands = [{
if (!param.startsWith('http')) {
param = 'http://' + param
}
remote.get(param)
fetchRemoteGrass(param)
Copy link
Member

Choose a reason for hiding this comment

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

Should the whitelist be passed as a param here?

@pe4cey
Copy link
Contributor Author

pe4cey commented Jun 28, 2017

@oskarhane updated

@oskarhane
Copy link
Member

lgtm

@oskarhane oskarhane merged commit 7b10138 into neo4j:3.0 Jun 29, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants