Guides: make images render, and stop drafts leaking to readers
Images were a broken icon for every user. The stored URL was bound straight to <img src>, and a plain image request carries no Authorization header while this app keeps its JWT in one, so every fetch was a 401. They are now fetched through the API client, which also fixes the stored '/api/...' path under a subpath mount where the API is not at /api at all. A failed image says so in words rather than rendering as a broken icon, and the editor's own thumbnails go through the same path - an author needs to see the screenshot they pasted. Revision and image routes gated on their parent guide. Both were gated on guides.view alone, and revision ids are sequential, so being able to read one guide was enough to walk the ids and read every unpublished draft on the site - the opposite of what docs/GUIDES.md promises. An image in a PUBLISHED guide still serves to any reader; one that only appears in a draft does not. The guide list, the tag chips and search now match the PUBLISHED text rather than the working copy. Matching the working copy handed readers a hit on wording that is not in the page when they open it. The slug no longer follows the title. It was regenerated on every save that carried a title and the editor autosaves every three seconds, so fixing a typo in a heading silently moved the URL and broke exactly the pasted links the stored slug exists to protect. publish() now aborts when the pre-publish save failed. It ran regardless, so the author saw "Published version N" while readers got the previous revision. Link rendering: text is escaped before formatting, so a captured URL arrives carrying entities and was escaped a second time - every multi-parameter ServiceNow and SharePoint link opened with &amp; in it. Emphasis could also reach inside an href, and '//host' passed as an in-app link when the browser reads it as off-site. Tests are now HTTP-level. The draft-invisibility rule lived entirely in routes that no test executed, which is how three of them shipped serving drafts.
This commit is contained in:
cproudlock
@@ -332,3 +332,230 @@ def test_a_corrupt_blocks_column_renders_as_empty_rather_than_erroring(gd_app, g
|
||||
guide.blocksjson = 'not json at all'
|
||||
_db.session.commit()
|
||||
assert guide.blocks == []
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# The API surface. Everything above this line tests services directly; the
|
||||
# draft-invisibility rule lives in the ROUTES, and until these existed no test
|
||||
# executed a single one of them. That is how three routes shipped serving
|
||||
# unpublished drafts to any reader who could count upwards through ids.
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
def _user_with(db, username, permissions, rolename=None):
|
||||
"""A user holding exactly these guides permissions and nothing else."""
|
||||
from werkzeug.security import generate_password_hash
|
||||
from shopdb.core.models import User, Role, Permission
|
||||
|
||||
role = Role(rolename=rolename or (username + 'role'), description=username)
|
||||
for name in permissions:
|
||||
# Shared across fixtures: two users in one test both want guides.view,
|
||||
# and permissions.name is unique.
|
||||
permission = Permission.query.filter_by(name=name).first()
|
||||
if permission is None:
|
||||
permission = Permission(name=name, description=name, category='guides')
|
||||
db.session.add(permission)
|
||||
db.session.flush()
|
||||
role.permissions.append(permission)
|
||||
db.session.add(role)
|
||||
db.session.flush()
|
||||
|
||||
user = User(username=username, email=username + '@test.local',
|
||||
passwordhash=generate_password_hash('testpass'))
|
||||
user.roles.append(role)
|
||||
db.session.add(user)
|
||||
db.session.commit()
|
||||
return user
|
||||
|
||||
|
||||
def _login(client, username):
|
||||
response = client.post('/api/auth/login',
|
||||
json={'username': username, 'password': 'testpass'})
|
||||
assert response.status_code == 200, response.get_json()
|
||||
return {'Authorization':
|
||||
'Bearer ' + response.get_json()['data']['access_token']}
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def reader(client, db):
|
||||
"""Can read guides. Cannot author them."""
|
||||
_user_with(db, 'guidereader', ['guides.view'])
|
||||
return _login(client, 'guidereader')
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def author(client, db):
|
||||
"""Can write their own guides, but holds no guides.edit over others'."""
|
||||
_user_with(db, 'guideauthor', ['guides.view', 'guides.create'])
|
||||
return _login(client, 'guideauthor')
|
||||
|
||||
|
||||
def _draft(db, title='Unreleased procedure', published=False):
|
||||
"""A guide with one saved revision, published or not."""
|
||||
from plugins.guides.models import Guide
|
||||
from plugins.guides.services import authoring
|
||||
guide = Guide(title=title, slug=title.lower().replace(' ', '-'),
|
||||
status='draft')
|
||||
db.session.add(guide)
|
||||
db.session.flush()
|
||||
guide, revision = authoring.save(
|
||||
guide, title=title,
|
||||
blocks=[_block('b1', 'paragraph', text='secret draft wording')])
|
||||
if published:
|
||||
authoring.publish(guide)
|
||||
db.session.commit()
|
||||
return guide, revision
|
||||
|
||||
|
||||
def test_a_reader_cannot_open_an_unpublished_guide_by_slug(client, db, reader):
|
||||
guide, _ = _draft(db)
|
||||
response = client.get('/api/guides/' + guide.slug, headers=reader)
|
||||
assert response.status_code == 404
|
||||
|
||||
|
||||
def test_a_reader_cannot_read_an_unpublished_revision_by_id(client, db, reader):
|
||||
"""Revision ids are sequential, so this route is walkable by hand.
|
||||
|
||||
Gating it on guides.view alone meant anyone who could read one guide could
|
||||
read every draft on the site by counting upwards.
|
||||
"""
|
||||
guide, revision = _draft(db)
|
||||
response = client.get('/api/guides/revisions/{}'.format(revision.guiderevisionid),
|
||||
headers=reader)
|
||||
assert response.status_code == 404
|
||||
assert 'secret draft wording' not in response.get_data(as_text=True)
|
||||
|
||||
|
||||
def test_a_reader_may_read_the_published_revision_by_id(client, db, reader):
|
||||
guide, revision = _draft(db, title='Published procedure', published=True)
|
||||
response = client.get('/api/guides/revisions/{}'.format(revision.guiderevisionid),
|
||||
headers=reader)
|
||||
assert response.status_code == 200
|
||||
|
||||
|
||||
def test_a_reader_cannot_list_a_guides_revision_history(client, db, reader):
|
||||
guide, _ = _draft(db, title='Published procedure', published=True)
|
||||
response = client.get('/api/guides/{}/revisions'.format(guide.guideid),
|
||||
headers=reader)
|
||||
assert response.status_code == 403
|
||||
|
||||
|
||||
def test_search_does_not_match_a_reader_against_draft_wording(client, db, reader):
|
||||
"""A hit a reader cannot open is worse than no hit.
|
||||
|
||||
The guide row holds the WORKING copy, so matching it would surface a
|
||||
published guide on a word that exists only in somebody's unpublished edit -
|
||||
and the word is not on the page when the reader opens it.
|
||||
"""
|
||||
from plugins.guides.services import authoring
|
||||
guide, revision = _draft(db, title='Published procedure', published=True)
|
||||
authoring.save(guide, blocks=[_block('b1', 'paragraph',
|
||||
text='unpublished sentence about widgets')])
|
||||
db.session.commit()
|
||||
|
||||
response = client.get('/api/guides?q=widgets', headers=reader)
|
||||
assert response.status_code == 200
|
||||
assert response.get_json()['data'] == []
|
||||
|
||||
# The author of the draft still finds their own work.
|
||||
response = client.get('/api/guides?q=secret', headers=reader)
|
||||
assert response.status_code == 200
|
||||
|
||||
|
||||
def test_an_author_may_edit_their_own_draft_but_not_someone_elses(client, db, author):
|
||||
guide, _ = _draft(db)
|
||||
response = client.get('/api/guides/{}/working'.format(guide.guideid),
|
||||
headers=author)
|
||||
assert response.status_code == 403, 'not their guide'
|
||||
|
||||
|
||||
def test_an_image_upload_refuses_a_type_that_is_not_an_image(client, db, author):
|
||||
import io
|
||||
data = {'file': (io.BytesIO(b'MZ not a picture'), 'payload.exe')}
|
||||
response = client.post('/api/guides/images', data=data, headers=author,
|
||||
content_type='multipart/form-data')
|
||||
assert response.status_code == 400
|
||||
assert 'Unsupported image type' in response.get_json()['data']['error']['message']
|
||||
|
||||
|
||||
def test_an_image_in_a_draft_is_not_served_to_a_reader(client, db, reader, author):
|
||||
"""An image is content, and a draft's images are draft content."""
|
||||
import io
|
||||
from plugins.guides.models import GuideImage
|
||||
|
||||
png = (b'\x89PNG\r\n\x1a\n' + b'0' * 32)
|
||||
upload = client.post(
|
||||
'/api/guides/images',
|
||||
data={'file': (io.BytesIO(png), 'screenshot.png')},
|
||||
headers=author, content_type='multipart/form-data')
|
||||
assert upload.status_code == 200, upload.get_json()
|
||||
imageid = upload.get_json()['data']['guideimageid']
|
||||
|
||||
# Unattached: it belongs to whoever uploaded it, not to every reader.
|
||||
assert client.get('/api/guides/images/{}'.format(imageid),
|
||||
headers=reader).status_code == 404
|
||||
assert client.get('/api/guides/images/{}'.format(imageid),
|
||||
headers=author).status_code == 200
|
||||
|
||||
# Attached to an unpublished guide: still not the reader's business.
|
||||
guide, _ = _draft(db)
|
||||
db.session.get(GuideImage, imageid).guideid = guide.guideid
|
||||
db.session.commit()
|
||||
assert client.get('/api/guides/images/{}'.format(imageid),
|
||||
headers=reader).status_code == 404
|
||||
|
||||
|
||||
def test_an_image_in_a_published_guide_is_served_and_hardened(client, db, reader, author):
|
||||
"""The other half: a published screenshot must actually reach the reader.
|
||||
|
||||
Guarding this route without checking the published case is how you trade a
|
||||
leak for every image on the site rendering as a broken icon.
|
||||
"""
|
||||
import io
|
||||
from plugins.guides.models import Guide, GuideImage
|
||||
from plugins.guides.services import authoring
|
||||
|
||||
png = (b'\x89PNG\r\n\x1a\n' + b'0' * 32)
|
||||
upload = client.post(
|
||||
'/api/guides/images',
|
||||
data={'file': (io.BytesIO(png), 'screenshot.png')},
|
||||
headers=author, content_type='multipart/form-data')
|
||||
imageid = upload.get_json()['data']['guideimageid']
|
||||
|
||||
guide = Guide(title='With a picture', slug='with-a-picture', status='draft')
|
||||
db.session.add(guide)
|
||||
db.session.flush()
|
||||
guide, revision = authoring.save(
|
||||
guide, title='With a picture',
|
||||
blocks=[{'id': 'b1', 'type': 'image', 'imageid': imageid,
|
||||
'url': '/api/guides/images/{}'.format(imageid)}])
|
||||
authoring.publish(guide)
|
||||
db.session.get(GuideImage, imageid).guideid = guide.guideid
|
||||
db.session.commit()
|
||||
|
||||
response = client.get('/api/guides/images/{}'.format(imageid), headers=reader)
|
||||
assert response.status_code == 200
|
||||
assert response.data == png
|
||||
# Uploaded bytes are attacker-influenced; they must be inert in this origin.
|
||||
assert response.headers.get('X-Content-Type-Options') == 'nosniff'
|
||||
assert 'Content-Security-Policy' in response.headers
|
||||
|
||||
|
||||
def test_renaming_a_guide_does_not_move_its_url(client, db, author):
|
||||
"""The stored slug exists so a pasted link keeps working.
|
||||
|
||||
The editor autosaves on a timer, so re-slugging on title change moved the
|
||||
URL while somebody was still typing the title.
|
||||
"""
|
||||
created = client.post('/api/guides',
|
||||
json={'title': 'Replacing a marker controler',
|
||||
'blocks': []}, headers=author)
|
||||
assert created.status_code == 200, created.get_json()
|
||||
guideid = created.get_json()['data']['guideid']
|
||||
slug = created.get_json()['data']['slug']
|
||||
|
||||
fixed = client.put('/api/guides/{}'.format(guideid),
|
||||
json={'title': 'Replacing a marker controller',
|
||||
'blocks': []}, headers=author)
|
||||
assert fixed.status_code == 200
|
||||
assert fixed.get_json()['data']['slug'] == slug, 'the URL must not move'
|
||||
assert fixed.get_json()['data']['title'] == 'Replacing a marker controller'
|
||||
|
||||
Reference in New Issue
Block a user