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

[core] Use Netlify function for feedback management #36472

Merged
merged 26 commits into from
Mar 27, 2023

Conversation

alexfauquette
Copy link
Member

@alexfauquette alexfauquette commented Mar 9, 2023

To be able to delete a message, we need to be the author. So to let the bot delete messages it has to write them himself

So instead of using webhook, this PR uses a call to the serverless function that send the message to slack using the API

Then it's feasible to delete messages. To do so I support to action:

  • Delete which removes the message
  • Save which send the message content to a spreadsheet and notify the user when done

@alexfauquette alexfauquette added the docs Improvements or additions to the documentation label Mar 9, 2023
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Mar 17, 2023
@mui-bot
Copy link

mui-bot commented Mar 17, 2023

Netlify deploy preview

https://deploy-preview-36472--material-ui.netlify.app/

Bundle size report

No bundle size changes

Generated by 🚫 dangerJS against b5ce5c9

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Mar 17, 2023
@alexfauquette alexfauquette marked this pull request as ready for review March 17, 2023 09:59
@zannager zannager requested a review from mnajdova March 17, 2023 11:28
@zannager zannager added the core Infrastructure work going on behind the scenes label Mar 17, 2023
@alexfauquette alexfauquette added the scope: docs-infra Specific to the docs-infra product label Mar 17, 2023
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Mar 21, 2023
@alexfauquette alexfauquette changed the title [core] Test Netlify function for feedback [core] Use Netlify function for feedback management Mar 27, 2023
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Mar 27, 2023
@alexfauquette alexfauquette merged commit 0ba2838 into mui:master Mar 27, 2023
@oliviertassinari
Copy link
Member

oliviertassinari commented Mar 30, 2023

Regarding the security, there are the current public env tokens, are these safe to be public?

  • SLACK_SIGNING_SECRET
  • SLACK_FEEDBACKS_TOKEN is this a dead token to remove?
  • SLACK_BOT_TOKEN
  • G_SHEET_TOKEN this one looks OK-ish, all the saved feedbacks are effectivement public, but it sounds fair. It works because the Google account has a very limited access to Google Drive.

@Janpot
Copy link
Member

Janpot commented Mar 31, 2023

They are defined in netlify, I don't believe we're leaking these anywhere where a user of the docs can access them. These ones are not safe to be public.

@oliviertassinari
Copy link
Member

oliviertassinari commented Mar 31, 2023

We need to include all the env variables with https://app.netlify.com/sites/material-ui/settings/env so the contributors from the community can run the build:

Screenshot 2023-03-31 at 10 59 45

https://docs.netlify.com/environment-variables/get-started/#sensitive-variable-policy

So they are accessible to anyone who opens a PR to log them.

@Janpot
Copy link
Member

Janpot commented Mar 31, 2023

Ah yes, I see now, that's a problem indeed, great catch. They seem to suggest we can exclude certain variables from untrusted builds but it's not 100% clear to me yet how to do that exactly. We should definitely do that if we can for the SLACK_SIGNING_SECRET and the G_SHEET_TOKEN.

According to the docs it seems we need to enable "Deploy without sensitive variables" then. But how do you mark variables as public/private then? Just erase the value for "Deploy Previews" context?

@alexfauquette
Copy link
Member Author

About what can be done with those tokens

  • SLACK_FEEDBACKS_TOKEN is needed for the mui-x v5 docs. It allows sending messages to #docs-feedback channel. That's all. So getting it could allow to spam us
  • SLACK_SIGNING_SECRET or SLACK_BOT_TOKEN would allows to fake requests from slack interaction, or directly use API calls from our bot. So they could read/write/delete messages. But bots are limited to the channels where they are invited
    https://api.slack.com/authentication/basics#public

So they are accessible to anyone who opens a PR to log them.

A solution is to restrict those tokens to production. And if needed for tests, we can still add them for the specific branch. Such as to log them, attackers should have their log in production

@oliviertassinari
Copy link
Member

oliviertassinari commented Mar 31, 2023

But bots are limited to the channels where they are invited https://api.slack.com/authentication/basics#public

Ok, then I think that we are good

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Infrastructure work going on behind the scenes docs Improvements or additions to the documentation scope: docs-infra Specific to the docs-infra product
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants