Azure / static-web-apps

Azure Static Web Apps. For bugs and feature requests, please create an issue in this repo. For community discussions, latest updates, kindly refer to the Discussions Tab. To know what's new in Static Web Apps, visit https://aka.ms/swa/ThisMonth
https://aka.ms/swa
MIT License
327 stars 56 forks source link

Pre-Production environments use same settings as production as default #86

Open nogic1008 opened 4 years ago

nogic1008 commented 4 years ago

By default, pre-production environment is automatically created when a pull request is submitted. However, it uses same settings as production like connection string to DB.

It is possible to attack the production environment from the pre-production environment by sending a pull request containing a malicious API.

annaji-msft commented 4 years ago

@dariagrigoriu sounds like a valid ask to keep the prod and stage site settings independent unless specified. Similar to slot settings that that could be sticky in app service and azure functions.

@nogic1008 did I understand this right?

nogic1008 commented 4 years ago

@anganti

@nogic1008 did I understand this right?

Exactly right. Currently, settings can only be changed after a pull request has been submitted.

lmaccherone commented 4 years ago

+1. A workaround might be to include both staging and production connection string environment variables but decide which to use in code... assuming you can sense which environment you are running in. It doesn't appear that NODE_ENV is automatically set to 'production'. The context.req.url gets changed to localhost regardless so that doesn't work. Ideas?

lmaccherone commented 4 years ago

It looks like process.env.WEBSITE_STATICWEBAPP_FUNCTION_ASSIGNEEID contains the string 'master' when in prod but contains the pull request number in the same spot when in PR staging. I'd still prefer something better.

nogic1008 commented 4 years ago

@lmaccherone There are off topic. I'm concerned about the risk of leaking env settings such as connection strings.


In current,

Therefore, the value of process.env will be read by sending a pull request to add the following API and accessing the API of the staging environment before administrator changes the value.

module.exports = async function (context, req) {
    context.res = {
        body: process.env
    };
}
lmaccherone commented 4 years ago

Ugh, you are right. I didn't think of that.

miwebst commented 4 years ago

Hi @nogic1008, I understand the concern here. Today its not anyone can just publish a PR to update your api, they need to be a contributor to your repository in order to access the api secret for deploying. So the scope should really be anyone who has contributor access to your repo.

As for why this is implemented that way, its to make the preview process as simple as possible. If we remove this feature then if you are using Functions with some app settings/secrets then after creating your PR it is unlikely they will work and you will need to go update the environment to see the results of your PR. Thats a pretty painful experience if you need to reconfigure your staging environment on every PR.

I'm wondering what the desired alternative would be. Would you prefer to disable PRs creating staging environments for your static web app?

nogic1008 commented 4 years ago

@miwebst In my app I changed as below:

I think you need to comment out the pull_request trigger in the generated workflow to make this feature an opt-in option.

miwebst commented 4 years ago

This is a feature that is cool and we want people to take advantage of. I don't know if I agree with the proposed security risk unless there is someone with contributor access to your repository that can't be trusted.

Also just curious, but couldn't the bad actor add the trigger back to the production workflow file and essentially opt you back in? I believe the opt-out mechanism would need to be done on the Static Web App side rather than on the Github Actions side.

nogic1008 commented 4 years ago

I didn't think about that possibility, hmm...

Is it possible to prepared another value as default instead of using a production environment variable? Currently it can only be changed after the staging environment has been deployed.

miwebst commented 4 years ago

The problem is that we dont know the significance of the value. For example, consider an application where the owner is using a Function to write some content to CosmosDb. The function will need access to the cosmos connection string and lets assume it will throw 500's if it is not present. If we try to insert some default value or don't copy over the production environments settings then the staging environment will be broken since the connection string will not be present and the function will throw 500's.

If contributors to the repository can't be trusted then we may need to add a mechanism for turning off staging environments for this static web app, essentially opt-out.

nogic1008 commented 4 years ago

@miwebst How about overrides settings like ASP.NET Core?

Example

PR #XXX environment uses { "message": "PR #XXX", "connectionString": "Development", "title": "Production" }

miwebst commented 4 years ago

Hey @nogic1008 this is great feedback! Maybe the general solution is to have some sort of stage environment settings, if you specify them then we wont use the production environment's settings when creating new staging environments.

mwaddell commented 3 years ago

The problem is that we dont know the significance of the value. For example, consider an application where the owner is using a Function to write some content to CosmosDb. The function will need access to the cosmos connection string and lets assume it will throw 500's if it is not present. If we try to insert some default value or don't copy over the production environments settings then the staging environment will be broken since the connection string will not be present and the function will throw 500's.

If contributors to the repository can't be trusted then we may need to add a mechanism for turning off staging environments for this static web app, essentially opt-out.

It's simple enough to turn off staging environments just by modifying the workflow to only trigger on "push" and not "pull_request" events. I think that a better approach for SWA to take is to provide a mechanism for updating the "Configuration Settings" as part of the push, so there's no need to go into the portal to modify those settings manually.

This could be as simple as adding a new app_environment: option to the action. For example:

with:
  azure_static_web_apps_api_token: ${{ secrets.MY_TOKEN }}
  repo_token: ${{ secrets.GITHUB_TOKEN }}
  action: "upload"
  app_location: "wwwroot"
  api_location: "api" 
  app_artifact_location: "build"
  app_environment:
    debug: "true"
    connstring: ${{ secrets.COSMOSDB_CONN }}

Any items you specify will overwrite what's there, anything you don't specify leaves it alone. If you need a separate staging/prod environment, you can split the action up into 2 separate ones for "push" vs "pull_request".

This way, once you have set up your SWA slot, you don't really ever have to go to the portal again and the history of your configuration changes is stored in Git (more of an IaC approach anyways).

Note: splitting up the auto-generated workflow into 2 separate build_and_deploy_job definitions, one for "if push" and one for "if pull_request" might solve a lot of the confusion that people have raised in various issues...

nogic1008 commented 3 years ago

Any updates?

itpropro commented 3 years ago

The idea with SWAs is that you should use a separate one for each stage. So if you have a application called "MyApp", just create one SWA called "MyApp-dev" and another one called "MyApp-prod" or however your naming looks like. Then you can easily create two GitHub Actions (or one with conditions of course) in the same repository and use one workflow for the dev branch and one for the main branch. If someone wants to merge a PR (e.g. a feature branch) on dev, the pre-prod environment for "MyApp-dev" gets created (which should have the same settings) and if you create a PR from dev to main, a PR on your "MyApp-prod" SWA gets created to review with the main settings. This way you don't have any issues with app settings and proper separation between dev and prod (or more) environments. You can easily adjust you workflows in a repository:

Workflow dev

on:
  push:
    branches:
      - dev
  pull_request:
    types: [opened, synchronize, reopened, closed]
    branches:
      - dev

Workflow main

on:
  push:
    branches:
      - main
  pull_request:
    types: [opened, synchronize, reopened, closed]
    branches:
      - main
thomasgauvin commented 1 year ago

We are looking into this 👀