Sapper: is it safe to render sensitive data, based on user rights?

user_4117781 0
Viewed 160

I want to render 500 error's traceback came from server only for admin, so:

  1. In server.js, session is populated with user retrieved from http_only cookie, just something like {'username': 'admin'}
polka()
    .use(
        sapper.middleware({
            session: (req, res) => {
                return { 'user': parseCookie('user') }
            }
        })
    )
    .listen(PORT);
  1. In some index.js there is a global variable to store possible traceback of 500 error came from server:
import { writable } from 'svelte/store';

export const error = writable();
  1. In index.html article is preloaded, and in case of 500 error, traceback is rendered below if current user is admin:
<script context="module">

    import { error } from 'index.js';

    export async function preload(page, session) {
        return { article : await this.fetch('/api/article/').then(response => {
            if (response.status == 500 && session.user.username === 'admin') {
                error.set(response);
            }
            return response.json();
        })}
    }
</script>

<script>
    export let article
</script>

<h1>{ article.title }</h1>
<div>{ article.text }</div>

<!-- 500 ERROR TRACEBACK --->
{#if $error}
    {@html $error}
{/if}

So, if $error is set via preload function, will it be safe and rendered only server-side? If not, how can it be improved? Maybe if (process.browser) could help somehow? Thx

sapper svelte
2 Answers

Avoid using stores like this.

Imagine that response.json() takes a few milliseconds to resolve, and in that time a new request happens that also errors. The first user is a regular user, the second user is an admin. Since the error store is common to all users who connect to that server, the first user would see the error meant for the admin user in that scenario.

Instead, just expose error as a prop:

<script context="module">
    export async function preload(page, session) {
        const response = await this.fetch('/api/article/');

        return {
            article: await response.json(),
            error: response.status === 500 && session.user.username === 'admin'
                ? response
                : null
        };
    }
</script>

<script>
    export let article;
    export let error;
</script>

<h1>{ article.title }</h1>
<div>{ article.text }</div>

<!-- 500 ERROR TRACEBACK --->
{#if error}
    {@html error}
{/if}

(To be more rigorous still, it's better to avoid using the session object in this way, since someone sufficiently dedicated could find a way to access that object and mutate it — the server should be responsible for determining what information to show the user, not the client. Though in this case it doesn't really matter since you're just displaying information that is easily accessible via the network tab.)

user_27423960

Update

Check the below answer by Rich Harris

Original

If you have used one of the templates with either webpack or rollup you will see that process.browser is replaced with true meaning that unreachable code will be tree shaked.

So short answer would be yes, you should be fine to use it as long as you surround that code with process.browser in appropriate place

But with that being said you are better off relying on server logs when it comes to 500 error code. Returning just error code to all of the users and nothing more, and pushing stack traces to log system instead for debugging.

user_36183870
Related