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

backport algolia search support from buildkite.com #681

Merged
merged 2 commits into from
Mar 31, 2020
Merged

Conversation

yob
Copy link
Contributor

@yob yob commented Mar 27, 2020

A few weeks ago, we added search to https://buildkite.com/docs, using algolia.

We're in the process of extracting the docs content to be hosted as a dedicated, self-contained rails app that's deployed to fargate. Before that migration can be made live for users, we first need to backport the new search functionality to this app so it'll remain available for users.

Most of this is copied as-is from its original home, and it seems to work.

I've opted to store the algolia config values in SSM for consistency. On the one hand their public values that can be seen in the page source, but on the other hand they're config and it feel weird to just store them in a view. I'm happy to reconsider if anyone feels strongly.

I also had to hack the CSP settings so the algolia JS would load. I haven't done much work with CSP so I assume I've opened a big security hole and would love someone to take a look at it.

A few weeks ago, we added search to https://buildkite.com/docs, using
algolia [1].

We're in the process of extracting the docs content to be hosted as a
dedicated, self-contained rails app that's deployed to fargate. Before
that migration can be made live for users, we first need to backport the
new search functionality to this app so it'll remain available for
users.

Most of this is copied as-is from its original home, and it seems to
work.

I've opted to store the algolia config values in SSM for consistency.
On the one hand their public values that can be seen in the page source,
but on the other hand they're config and it feel weird to just store
them in a view. I'm happy to reconsider if anyone feels strongly.

I also had to hack the CSP settings so the algolia JS would load. I
haven't done much work with CSP so I assume I've opened a big security
hole and would love someone to take a look at it.

[1] https://www.algolia.com/
@yob yob requested review from toolmantim and keithpitt March 27, 2020 12:36
@harrietgrace harrietgrace temporarily deployed to docs-review-backport-se-dm1fgb March 27, 2020 12:37 Inactive
@yob
Copy link
Contributor Author

yob commented Mar 27, 2020

Tagging @keithpitt (for algolia experience) and @toolmantim (for CSP wizardry)

#policy.script_src :self, :unsafe_eval, Matomo::URL, :unsafe_inline, :strict_dynamic, :report_sample, :https, :http
policy.script_src :unsafe_eval, :unsafe_inline, :strict_dynamic, :report_sample, :https, :http
end
policy.connect_src 'https://*.algolia.net', 'https://*.algolianet.com'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I assume my changes here are wrong 🙈

Copy link
Contributor

Choose a reason for hiding this comment

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

That looks okay to me? Anything that you’re particularly worried about?

Copy link
Contributor

Choose a reason for hiding this comment

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

I’m spin this up and see if we can find a way to have a stricter CSP, given how tiny the docs app is.

Copy link
Contributor Author

@yob yob Mar 29, 2020

Choose a reason for hiding this comment

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

Anything that you’re particularly worried about?

Just that I don't understand CSP. I can see a lot of care went into crafting the CSP rules on buildkite.com, and the rules that I cargo-culted into here had very little care put into them.

I’m spin this up and see if we can find a way to have a stricter CSP

I'm not 100% confident what makes a CSP more or less strict. Would you have time for a quick pairing session on this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, sounds good @yob!

@toolmantim
Copy link
Contributor

Amazing @yob! ❤️

#policy.script_src :self, :unsafe_eval, Matomo::URL, :unsafe_inline, :strict_dynamic, :report_sample, :https, :http
policy.script_src :unsafe_eval, :unsafe_inline, :strict_dynamic, :report_sample, :https, :http
end
policy.connect_src 'https://*.algolia.net', 'https://*.algolianet.com'
Copy link
Contributor

Choose a reason for hiding this comment

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

I’m spin this up and see if we can find a way to have a stricter CSP, given how tiny the docs app is.

Tim and I just spent some time pairing on the best way to get algolia
backported from bk/bk to this app.

We explored leaving CSP enforced, but that requires allowing
`unsafe_eval` in production and that's not ideal. It turns out CSP on
bk/bk is in report-only mode as well, and docsearch is violating it
(with a report) each time someone searches.

So for now, we've configured this appto behave the same way.

We'd love to start enforcing CSP on the docs app, but there's been
little movement on docsearch adapting to be more CSP friendly (see
algolia/docsearch#773).

Algolia have an alternative JS library that is CSP friendly and we'd
love to use it.  Maybe that's our best path to enforcing CSP?

https://www.algolia.com/doc/guides/building-search-ui/what-is-instantsearch/js/
@harrietgrace harrietgrace temporarily deployed to docs-review-backport-se-dm1fgb March 31, 2020 01:16 Inactive
@yob
Copy link
Contributor Author

yob commented Mar 31, 2020

@toolmantim and I just spent some time pairing on the best way to get algolia backported from bk/bk to this app without violating CSP rules.

We explored leaving CSP enforced, but that requires allowing unsafe_eval in production and that's not ideal. It turns out CSP on bk/bk is in report-only mode as well, and docsearch is violating it (with a report) each time someone searches.

So for now, we've configured this appto behave the same way.

We'd love to start enforcing CSP on the docs app, but there's been little movement on docsearch adapting to be more CSP friendly (see algolia/docsearch#773).

Algolia have an alternative JS library that is CSP friendly and we'd love to use it. Maybe that's our best path to enforcing CSP? https://www.algolia.com/doc/guides/building-search-ui/what-is-instantsearch/js/

Copy link
Contributor

@toolmantim toolmantim left a comment

Choose a reason for hiding this comment

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

InstantSearch.js is something @scopegate has been working with recently too, so there's definitely a path forward there with an enforced CSP I reckon 👌🏼

@yob yob merged commit c13905f into master Mar 31, 2020
@yob yob deleted the backport-search branch March 31, 2020 12:39
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.

3 participants