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

Use route-level config to set allowedRoles #134

Open
geoffrich opened this issue Mar 22, 2023 · 2 comments
Open

Use route-level config to set allowedRoles #134

geoffrich opened this issue Mar 22, 2023 · 2 comments

Comments

@geoffrich
Copy link
Owner

Related: #65, #60, #66, #102

The problem: Azure SWA allows you to secure routes via setting allowedRoles in the routes config. However, it is not easy to take advantage of this feature with this SvelteKit adapter.

  • you can't set an allowed role for all routes, since the adapter prevents overriding the "*" route
  • to set an allowed role for other routes that you also want handled by SvelteKit, you have to manually add a rewrite to the SSR function, which is technically private API

The proposal: use SvelteKit's route-level config to associate allowedRoles with a route. This could be extended for other route-level configuration in the future, if needed.

For example, if you have a route at /secured, you could set allowedRoles in the corresponding +page.js or +page.server.js file like so:

export const config = {
  allowedRoles: ['admin']
}

The adapter would use this information to write the following route rule:

{
  "route": "/secured",
  "allowedRoles": ["admin"],
  "rewrite": "/api/__render" 
}

You could also export config from a +layout.js, and the adapter would write a route with an allowed role for each route under that layout.

This solves the aforementioned issues:

  • you can set an allowed role for all routes (Unable to set wildcard route #65) by exporting config from a root-level +layout.js.
  • you no longer have to set rewrite yourself, and renaming your route folders in SvelteKit will automatically update the config (routes and config stay in sync)

It also has several more benefits

  • we can automatically protect __data.json routes as well, which are generated by SvelteKit to fetch data

Passing routes to the adapter would still be supported as it is today. Those routes would appear before any routes generated by the adapter, so to have priority.

Technical breakdown

During the adapter step, SvelteKit provides a routes object representing all the routes in the app and the config associated with those routes. For example:

[
  {
    id: '/sverdle/how-to-play',
    api: { methods: [] },
    page: { methods: [Array] },
    segments: [ [Object], [Object] ],
    pattern: /^\/sverdle\/how-to-play\/?$/,
    prerender: true,
    methods: [ 'GET' ],
    config: { foo: 'bar' }
  },
  {
    id: '/[slug]/[anotherslug]',
    api: { methods: [] },
    page: { methods: [Array] },
    segments: [ [Object], [Object] ],
    pattern: /^\/([^/]+?)\/([^/]+?)\/?$/,
    prerender: false,
    methods: [ 'GET' ],
    config: { foo: 'bar' }
  }
]

The types for RouteDefinition are in the SvelteKit repo

We can convert the pattern regex into the route property needed by the SWA config. (We can't use route.id since it could include layout groups or optional parameters). Then, for each route, write a rule with the route pattern and the specified allowedRoles. We may also want to write a rule for the __data.json endpoint associated with the route.

One sticking point is the relative inflexibility of Azure SWA route pattern. The wildcard can only appear at the end of the route pattern, so it wouldn't be possible to protect /blog/[slug]/edit but not /blog/[slug] - they would both be represented by the /blog/* pattern. We would need to detect this conflict at build time and throw an error.

Also, because route config applies to an entire route folder, you couldn't protect an individual slug with this method - either you protect the entire /blog/[slug] route, or none of it. You could get around this by passing custom routes to the adapter, but then you would have to handle the rewrite property yourself.

We also may want to warn/error if a user-supplied route does not set allowedRoles, since that will override the route generated by the adapter.

Limitations

There are some limitations:

  • the SWA config file has a max size of 20KB. Apps with a large number of protected routes could hit this restriction, since we'll be writing an object for each unique route. There may be ways we can merge rules, e.g. if all routes under a given route segment are protected, then write a single wildcard rule
  • as mentioned in the previous section, dynamic routes are limited to what can be expressed with SWA's wildcard syntax
  • using this auth at dev time may be tricky (see Investigate improving SWA CLI story #96, Investigate Static Web App Auth + SvelteKit #102). Even with SWA CLI working, you will need to re-generate the config whenever you change the allowedRoles.
  • while you can protect static/prerendered pages with this method, they will still be accessible if you client-side route to them and may appear in the bundle. We'll want to document this carefully (see also Investigate Static Web App Auth + SvelteKit #102 (comment)). The SWA docs call out this limitation as well:

Routing rules can only secure HTTP requests to routes that are served from Static Web Apps. Many front-end frameworks use client-side routing that modifies routes in the browser without issuing requests to Static Web Apps. Routing rules don't secure client-side routes. Clients should call HTTP APIs to retrieve sensitive data. Ensure APIs validate a user's identity before returning data.

Potential future enhancements

I wouldn't include these in the initial release of this feature, but here are some potential improvements:

  • protecting individual methods (e.g. require admin role for POST but not GET). Though I suspect it would be better to enforce this at runtime in the app itself instead of via config.
  • automatically transform custom route config passed to the adapter (e.g. to allow protecting individual slugs in the /blog/[slug] example above). One idea: if you pass a route handled by the SvelteKit app (determined using the pattern property in the adapter) but don't rewrite or redirect it, automatically add the rewrite rule

Any questions?

Would love to hear feedback about this approach? Is this workable? Is there anything I missed?

@FlippingBinary
Copy link
Contributor

That sounds like a great solution! Here are some thoughts:

  • It might actually be easier to parse route.id instead of regex patterns when identifying routes, because the syntax for layout groups and optional parameters is well-defined and less complex. Parsing regex patterns can be error-prone.
  • The adapter could build an internal table of routes with tentative SWA route names for each SvelteKit route, and throw an error if it encounters multiple SvelteKit routes with contradictory rules that translate to the same SWA route. It could then optimize the table by combining multiple SvelteKit routes with identical rules into fewer wildcard SWA routes when possible, resulting in a smaller config file and reducing the risk of hitting the 20KB file size limit. If all the routes have the same allowedRoles, for example, the output should be a single wildcard route (aside from the ones for the adapter, of course). SWA uses the first matching rule, so the most common sets of rules could convert to wildcard routes while the rest are listed above them.
  • The legacy method of specifying routes in svelte.conf.js could be merged into the internal table described above (with higher priority) to confirm it does not produce any routing errors and report them if it does. This error reporting might also identify the specific options that are available to solve the conflict.
  • If the user doesn't set allowedRoles for a route, they may have set it at the top level of the config object to apply to all nested routes, or the website might be publicly-accessible intentionally. The config object merging rules might need some explicit examples to avoid confusion, but I'm not sure if an error would be appropriate. The routes without allowedRoles should just inherit the allowedRoles of the top-level +layout.ts, if there is one. Perhaps the adapter could require all routes to specify their allowedRoles if even one route does?
  • The config structure could be modified to allow for specific slug values, which would enable protection of individual slugs without falling back to the older svelte.conf.js way of doing it. It could also include a way of defining which methods the roles should be enforced on. The routes data structure appears to specify the methods that apply to a route, but this would allow more granularity in defining role restrictions when a route accepts both GET and PUT, for example.
    export const config = {
      slugs: {
        '[action]': {
          'edit': {
            methods: ['POST'],
            allowedRoles: ['admin']
          }
        }
      }
      allowedRoles: ['authenticated']
    }
    Or it could be nested under config.allowedRoles and arrays could be interpreted as a list of allowed roles while objects would be a more complex structure like this:
    export const config = {
      allowedRoles: {
        'GET': ['authenticated'],
        'PUT': ['admin']
      }
    }
    Or slightly more complex to allow for different rules for different slugs (this would be compatible with the second example above):
    export const config = {
      allowedRoles: {
        '[action]': {
          'edit': {
            'GET': ['authenticated'],
            'PUT': ['admin']
          }
          '*': ['anonymous']
        }
      }
    }
    More specific rules would override less specific rules because that's the most natural. This idea might need some work, but that's what popped into my head.
  • It would be helpful to have a way to detect whether routes link to other routes with different protection rules that don't enforce SSR/static generation. Is there a data structure available to the adapter that contains this information? If so, the adapter could throw an error and refuse to build in such cases because the client-side rendering would leak protected content.

That's what I've got so far.

@geoffrich
Copy link
Owner Author

Thanks for the feedback!

If the user doesn't set allowedRoles for a route, they may have set it at the top level of the config object to apply to all nested routes, or the website might be publicly-accessible intentionally. The config object merging rules might need some explicit examples to avoid confusion, but I'm not sure if an error would be appropriate. The routes without allowedRoles should just inherit the allowedRoles of the top-level +layout.ts, if there is one. Perhaps the adapter could require all routes to specify their allowedRoles if even one route does?

Not sure I follow the case this is talking about, but if the user sets allowedRoles in the layout's config, then it should automatically apply to all child pages without them also specifying allowedRoles. This is just how SvelteKit's page options work.

It would be helpful to have a way to detect whether routes link to other routes with different protection rules that don't enforce SSR/static generation. Is there a data structure available to the adapter that contains this information? If so, the adapter could throw an error and refuse to build in such cases because the client-side rendering would leak protected content.

Hm, interesting thought. I'm not aware of any data structure, and such a data structure wouldn't be complete since links can be dynamic. Even if there was, an attacker could theoretically still access the content by inspecting the bundle. It might be best to warn (or error?) if we detect a prerendered page with allowedRoles set, regardless of if it's linked to.

Re: the proposed API for allowedRoles, this kind of API definitely makes sense to me:

export const config = {
  allowedRoles: {
    'GET': ['authenticated'],
    'PUT': ['admin']
  }
}

// or allowedRoles: ['admin'] if all methods

The slug versions are kinda hard for me to parse, might need to think a bit more on what a good API there would be. It does seem like a rather niche usecase too, so maybe it doesn't need to be solved immediately as long as we can introduce it without a breaking change.

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

No branches or pull requests

2 participants