Skip to content

Commit

Permalink
🔒 Fixed some security issues. Please upgrade!
Browse files Browse the repository at this point in the history
  • Loading branch information
mawoka-myblock committed Jan 21, 2024
1 parent e8e330e commit 188a935
Show file tree
Hide file tree
Showing 12 changed files with 86 additions and 48 deletions.
2 changes: 2 additions & 0 deletions classquiz/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -102,4 +102,6 @@ def settings() -> Settings:

ALLOWED_TAGS_FOR_QUIZ = ["b", "strong", "i", "em", "small", "mark", "del", "sub", "sup"]

ALLOWED_MIME_TYPES = ["image/png", "video/mp4", "image/jpeg", "image/gif", "image/webp"]

server_regex = rf"^{re.escape(settings().root_address)}/api/v1/storage/download/.{{36}}--.{{36}}$"
6 changes: 3 additions & 3 deletions classquiz/routers/editor.py
Original file line number Diff line number Diff line change
Expand Up @@ -72,10 +72,10 @@ async def finish_edit(edit_id: str, quiz_input: QuizInput):
if session_data is None:
raise HTTPException(status_code=401, detail="Edit ID not found!")
session_data = EditSessionData.parse_raw(session_data)
quiz_input.title = html.unescape(bleach.clean(quiz_input.title, tags=ALLOWED_TAGS_FOR_QUIZ, strip=True))
quiz_input.description = html.unescape(bleach.clean(quiz_input.description, tags=ALLOWED_TAGS_FOR_QUIZ, strip=True))
quiz_input.title = bleach.clean(quiz_input.title, tags=ALLOWED_TAGS_FOR_QUIZ, strip=True)
quiz_input.description = bleach.clean(quiz_input.description, tags=ALLOWED_TAGS_FOR_QUIZ, strip=True)
if quiz_input.background_color is not None:
quiz_input.background_color = html.unescape(bleach.clean(quiz_input.background_color, tags=[], strip=True))
quiz_input.background_color = bleach.clean(quiz_input.background_color, tags=[], strip=True)

for i, question in enumerate(quiz_input.questions):
if question.type == QuizQuestionType.ABCD:
Expand Down
6 changes: 3 additions & 3 deletions classquiz/routers/quiztivity/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,21 +17,21 @@
router.include_router(shares_router, prefix="/shares")


@router.post("/create")
@router.post("/create", response_model_exclude={"user": ...})
async def create_quiztivity(data: QuizTivityInput, user: User = Depends(get_current_user)) -> QuizTivity:
quiztivity = QuizTivity.parse_obj({**data.dict(), "user": user, "id": uuid4(), "created_at": datetime.now()})
return await quiztivity.save()


@router.get("/{uuid}")
@router.get("/{uuid}", response_model_exclude={"user": ...})
async def get_quiztivity(uuid: UUID) -> QuizTivity:
quiztivity = await QuizTivity.objects.get_or_none(id=uuid)
if quiztivity is None:
raise HTTPException(status_code=404, detail="QuizTivity not found")
return quiztivity


@router.put("/{uuid}")
@router.put("/{uuid}", response_model_exclude={"user": ...})
async def put_quiztivity(data: QuizTivityInput, uuid: UUID, user: User = Depends(get_current_user)) -> QuizTivity:
quiztivity = await QuizTivity.objects.get_or_none(id=uuid, user=user)
if quiztivity is None:
Expand Down
5 changes: 3 additions & 2 deletions classquiz/routers/storage.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
from pydantic import BaseModel

from classquiz.auth import get_current_user
from classquiz.config import settings, storage, arq
from classquiz.config import settings, storage, arq, ALLOWED_MIME_TYPES
from classquiz.db.models import User, StorageItem, PublicStorageItem, UpdateStorageItem, PrivateStorageItem
from classquiz.helpers import check_image_string
from classquiz.storage.errors import DownloadingFailedError
Expand Down Expand Up @@ -119,10 +119,11 @@ async def download_file_head(file_name: str) -> Response:

@router.post("/")
async def upload_file(file: UploadFile = File(), user: User = Depends(get_current_user)) -> PublicStorageItem:
if file.content_type not in ALLOWED_MIME_TYPES:
raise HTTPException(status_code=422, detail="Unsupported")
if user.storage_used > settings.free_storage_limit:
raise HTTPException(status_code=409, detail="Storage limit reached")
file_id = uuid4()

size = 0
file_obj = StorageItem(
id=file_id,
Expand Down
3 changes: 1 addition & 2 deletions classquiz/routers/users/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
from fastapi.background import BackgroundTasks
from fastapi.responses import JSONResponse, RedirectResponse, PlainTextResponse
from fastapi.security import OAuth2PasswordRequestForm
import html

from jose import jwt, JWTError

Expand Down Expand Up @@ -78,7 +77,7 @@ async def create_user(user: RouteUser, background_task: BackgroundTasks) -> User
raise HTTPException(status_code=409, detail="User already exists")

user.password = get_password_hash(user.password)
user.username = html.unescape(bleach.clean(user.username, tags=[], strip=True))
user.username = bleach.clean(user.username, tags=[], strip=True)
if len(user.username) == 32:
return JSONResponse({"details": "Username mustn't be 32 characters long"}, 400)
await user.save()
Expand Down
25 changes: 19 additions & 6 deletions classquiz/routers/users/twofa.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,10 @@
import urllib.parse

import pyotp
from fastapi import APIRouter, Depends
from fastapi import APIRouter, Depends, HTTPException
from pydantic import BaseModel

from classquiz.auth import get_current_user
from classquiz.auth import get_current_user, verify_password
from classquiz.db.models import User

router = APIRouter()
Expand All @@ -20,22 +20,31 @@ class GetBackupCodeResponse(BaseModel):
code: str


@router.get("/backup_code", response_model=GetBackupCodeResponse)
async def get_backup_code(user: User = Depends(get_current_user)):
class RequirePasswordForAction(BaseModel):
password: str


@router.post("/backup_code", response_model=GetBackupCodeResponse)
async def get_backup_code(data: RequirePasswordForAction, user: User = Depends(get_current_user)):
backup_code = os.urandom(32).hex()
user = await User.objects.get(id=user.id)
if not verify_password(data.password, user.password):
raise HTTPException(status_code=401, detail="Invalid")
user.backup_code = backup_code
await user.update()
return GetBackupCodeResponse(code=backup_code)


class SetRequirePassword(BaseModel):
require_password: bool
password: str


@router.post("/require_password", response_model=SetRequirePassword)
async def set_require_password(data: SetRequirePassword, user: User = Depends(get_current_user)):
user = await User.objects.get(id=user.id)
if not verify_password(data.password, user.password):
raise HTTPException(status_code=401, detail="Invalid")
user.require_password = data.require_password
await user.update()
return data
Expand All @@ -47,8 +56,10 @@ class SetTotpUpResponse(BaseModel):


@router.post("/totp", response_model=SetTotpUpResponse)
async def set_totp_up(user: User = Depends(get_current_user)):
async def set_totp_up(data: RequirePasswordForAction, user: User = Depends(get_current_user)):
user = await User.objects.get(id=user.id)
if not verify_password(data.password, user.password):
raise HTTPException(status_code=401, detail="Invalid")
user.totp_secret = pyotp.random_base32()
url = pyotp.totp.TOTP(user.totp_secret).provisioning_uri(
name=urllib.parse.quote(user.username), issuer_name="ClassQuiz"
Expand All @@ -71,7 +82,9 @@ async def get_totp_status(user: User = Depends(get_current_user)):


@router.delete("/totp")
async def disable_totp(user: User = Depends(get_current_user)):
async def disable_totp(data: RequirePasswordForAction, user: User = Depends(get_current_user)):
user = await User.objects.get(id=user.id)
if not verify_password(data.password, user.password):
raise HTTPException(status_code=401, detail="Invalid")
user.totp_secret = None
await user.update()
16 changes: 12 additions & 4 deletions classquiz/routers/users/webauthn.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
from pydantic import BaseModel
from webauthn.helpers.cose import COSEAlgorithmIdentifier

from classquiz.auth import get_current_user
from classquiz.auth import get_current_user, verify_password
from classquiz.db.models import User, FidoCredentials
from classquiz.config import redis, settings

Expand All @@ -28,9 +28,15 @@
router = APIRouter()


@router.get("/add_key", response_model=PublicKeyCredentialCreationOptions)
async def request_add_key_data(user: User = Depends(get_current_user)):
class RequirePasswordForAction(BaseModel):
password: str


@router.post("/add_key_init", response_model=PublicKeyCredentialCreationOptions)
async def request_add_key_data(data: RequirePasswordForAction, user: User = Depends(get_current_user)):
user = await User.objects.select_related("fidocredentialss").get(id=user.id)
if not verify_password(data.password, user.password):
raise HTTPException(status_code=401, detail="Invalid")
options = generate_registration_options(
rp_id=urllib.parse.urlparse(settings.root_address).hostname,
rp_name="ClassQuiz",
Expand Down Expand Up @@ -86,7 +92,9 @@ async def list_security_keys(user: User = Depends(get_current_user)):


@router.delete("/key/{key_id}")
async def delete_security_key(key_id: int, user: User = Depends(get_current_user)):
async def delete_security_key(data: RequirePasswordForAction, key_id: int, user: User = Depends(get_current_user)):
if not verify_password(data.password, user.password):
raise HTTPException(status_code=401, detail="Invalid")
key = await FidoCredentials.objects.get_or_none(pk=key_id, user=user.id)
if key is None:
raise HTTPException(status_code=404, detail="Key not found")
Expand Down
6 changes: 2 additions & 4 deletions frontend/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@
"felte": "^1.2.7",
"fuse.js": "^6.6.2",
"highlight.js": "^11.7.0",
"i18next": "^22.4.15",
"i18next-browser-languagedetector": "^7.0.1",
"js-cookie": "^3.0.1",
"jws": "^4.0.0",
Expand Down Expand Up @@ -104,8 +105,5 @@
"vite-plugin-iso-import": "^1.0.0",
"yup": "^1.1.1"
},
"type": "module",
"dependencies": {
"i18next": "^22.4.15"
}
"type": "module"
}
12 changes: 6 additions & 6 deletions frontend/pnpm-lock.yaml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion frontend/src/lib/i18n/locales/de.json
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,7 @@
"abcd_description": "Nur eine Antwort kann ausgewählt werden",
"voting_description": "Antworten geben keine Punkte",
"order_description": "Antworten müssen in die richtige Reihenfolge gebracht werden",
"text_description": "Spieler können text eingeben",
"text_description": "Spieler können Text eingeben",
"range_description": "Ein Zahlenbereich kann mit einem Schieberegler ausgewählt werden",
"check_choice_description": "Alle richtigen Antworten müssen für Punkte ausgewählt werden",
"need_more_help": "Benötigst du mehr Hilfe?",
Expand Down
32 changes: 24 additions & 8 deletions frontend/src/routes/account/settings/security/+page.svelte
Original file line number Diff line number Diff line change
Expand Up @@ -36,18 +36,25 @@ SPDX-License-Identifier: MPL-2.0
if (!browser || user_data?.require_password === undefined) {
return;
}
const pw = require_password()
const res = await fetch('/api/v1/users/2fa/require_password', {
method: 'POST',
headers: {
'Content-Type': 'application/json'
},
body: JSON.stringify({ require_password: user_data?.require_password })
body: JSON.stringify({ require_password: user_data?.require_password, password: pw })
});
user_data.require_password = (await res.json()).require_password;
};
const require_password = (): string => {
return prompt("Please enter your password to continue")
}
const add_security_key = async () => {
const res1 = await fetch('/api/v1/users/webauthn/add_key');
const pw = require_password()
const res1 = await fetch('/api/v1/users/webauthn/add_key_init', {method: "POST", body: JSON.stringify({password: pw}), headers: { 'Content-Type': 'application/json' }});
if (res1.status === 401) {alert("Password probably wrong"); return}
if (!res1.ok) {
throw Error('Response not ok');
}
Expand All @@ -74,29 +81,36 @@ SPDX-License-Identifier: MPL-2.0
};
const remove_security_key = async (key_id: number) => {
await fetch(`/api/v1/users/webauthn/key/${key_id}`, { method: 'DELETE' });
const pw = require_password()
const res = await fetch(`/api/v1/users/webauthn/key/${key_id}`, { method: 'DELETE', body: JSON.stringify({password: pw}), headers: { 'Content-Type': 'application/json' } });
if (res.status === 401) {alert("Password probably wrong"); return}
data = get_data();
};
const disable_totp = async () => {
await fetch(`/api/v1/users/2fa/totp`, { method: 'DELETE' });
const pw = require_password()
const res = await fetch(`/api/v1/users/2fa/totp`, { method: 'DELETE', body: JSON.stringify({password: pw}), headers: { 'Content-Type': 'application/json' } });
if (res.status === 401) {alert("Password probably wrong"); return}
data = get_data();
};
const enable_totp = async () => {
const res = await fetch('/api/v1/users/2fa/totp', { method: 'POST' });
const pw = require_password()
const res = await fetch('/api/v1/users/2fa/totp', { method: 'POST', body: JSON.stringify({password: pw}), headers: { 'Content-Type': 'application/json' } });
if (res.status === 401) {alert("Password probably wrong"); return}
data = get_data();
totp_data = await res.json();
};
const get_backup_code = async () => {
const pw = require_password()
if (!confirm('If you continue, your old backup-code will be removed.')) {
return;
}
const res = await fetch('/api/v1/users/2fa/backup_code');
const res = await fetch('/api/v1/users/2fa/backup_code', { method: 'POST', body: JSON.stringify({password: pw}), headers: { 'Content-Type': 'application/json' } });
if (res.status === 401) {alert("Password probably wrong"); return}
backup_code = (await res.json()).code;
};
$: console.log(user_data?.require_password, 'hello');
</script>

{#await data}
Expand All @@ -116,11 +130,12 @@ SPDX-License-Identifier: MPL-2.0
</div>
<div class="h-full w-full">
<h2 class="text-center text-2xl">{$t('security_settings.activate_2fa')}</h2>
<div class="flex h-full w-full justify-center flex-col">
<div class="flex h-full w-full justify-center flex-col" class:pointer-events-none={!totp_activated} class:grayscale={!totp_activated} class:opacity-50={!totp_activated}>
<div class="m-auto">
{#if user_data.require_password}
<div class="flex items-center space-x-2">
<button
disabled={!totp_activated}
on:click={() => {
user_data.require_password = !user_data.require_password;
save_password_required();
Expand All @@ -142,6 +157,7 @@ SPDX-License-Identifier: MPL-2.0
{:else}
<div class="flex items-center space-x-2">
<button
disabled={!totp_activated}
type="button"
on:click={() => {
user_data.require_password = !user_data.require_password;
Expand Down
Loading

0 comments on commit 188a935

Please sign in to comment.