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

Don't show foreign key links to tables the user cannot access #2178

Closed
simonw opened this issue Sep 7, 2023 · 5 comments
Closed

Don't show foreign key links to tables the user cannot access #2178

simonw opened this issue Sep 7, 2023 · 5 comments

Comments

@simonw
Copy link
Owner

simonw commented Sep 7, 2023

Spotted this problem while working on this plugin:

It's possible to make a table public to any users - but then you may end up with situations like this:

CleanShot 2023-09-07 at 10 55 02@2x

That table is public, but the foreign key links go to tables that are NOT public.

We're also leaking the names of the values in those private tables here, which we shouldn't do. So this is a tiny bit of an information leak.

Since this only affects people who have configured a table to be public that has foreign keys to a table that is private I don't think this is worth issuing a vulnerability report about - I very much doubt anyone is running Datasette configured in a way that could result in problems because of this.

@simonw
Copy link
Owner Author

simonw commented Sep 7, 2023

Relevant code:

if columns_to_expand:
expanded_labels = {}
for fk, _ in expandable_columns:
column = fk["column"]
if column not in columns_to_expand:
continue
if column not in columns:
continue
expanded_columns.append(column)
# Gather the values
column_index = columns.index(column)
values = [row[column_index] for row in rows]
# Expand them
expanded_labels.update(
await datasette.expand_foreign_keys(
database_name, table_name, column, values
)
)

Which calls this undocumented method:

datasette/datasette/app.py

Lines 938 to 973 in fbcb103

async def expand_foreign_keys(self, database, table, column, values):
"""Returns dict mapping (column, value) -> label"""
labeled_fks = {}
db = self.databases[database]
foreign_keys = await db.foreign_keys_for_table(table)
# Find the foreign_key for this column
try:
fk = [
foreign_key
for foreign_key in foreign_keys
if foreign_key["column"] == column
][0]
except IndexError:
return {}
label_column = await db.label_column_for_table(fk["other_table"])
if not label_column:
return {(fk["column"], value): str(value) for value in values}
labeled_fks = {}
sql = """
select {other_column}, {label_column}
from {other_table}
where {other_column} in ({placeholders})
""".format(
other_column=escape_sqlite(fk["other_column"]),
label_column=escape_sqlite(label_column),
other_table=escape_sqlite(fk["other_table"]),
placeholders=", ".join(["?"] * len(set(values))),
)
try:
results = await self.execute(database, sql, list(set(values)))
except QueryInterrupted:
pass
else:
for id, value in results:
labeled_fks[(fk["column"], id)] = value
return labeled_fks

@simonw
Copy link
Owner Author

simonw commented Sep 7, 2023

Should I put the permission check in that undocumented datasette.expand_foreign_keys() method? I think so - it should accept request.actor as one of its arguments.

@simonw simonw closed this as completed in dbfad6d Sep 7, 2023
@simonw
Copy link
Owner Author

simonw commented Sep 7, 2023

I ran this:

datasette content.db -p 8043 -m fk-auth.yml --root

Against this YAML:

databases:
  content:
    tables:
      users:
        allow:
          id: root

And it worked as it should - here's a screenshot of an anonymous user and a root user viewing the same page:

CleanShot 2023-09-07 at 16 05 34@2x

@simonw
Copy link
Owner Author

simonw commented Sep 7, 2023

This fix didn't work on Datasette Cloud. I used /-/permissions to debug it and saw this:

image

Only checking view-table is not enough: for my instance on Datasette Cloud the view permission check that should have failed was for the database or instance.

@simonw simonw reopened this Sep 7, 2023
@simonw
Copy link
Owner Author

simonw commented Sep 7, 2023

To test that locally, use this YAML instead:

databases:
  content:
    allow:
      id: root
    tables:
      releases:
        allow: true

And:

allow:
  id: root
databases:
  content:
    tables:
      releases:
        allow: true

@simonw simonw closed this as completed in c263704 Sep 7, 2023
simonw added a commit that referenced this issue Sep 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant