Django 프로젝트 설정 값에 환경변수를 사용하여 보안 강화 by hepheir · Pull Request #4 · tle-ko/auth-service

hepheir · 2025-11-29T11:05:14Z

환경변수 기반의 설정값 지정, 환경변수를 가져오는 방식에 대한 구현이 주요 변경사항입니다.

…져오는 함수 추가

…일에서 읽어오도록 기능 추가

hepheir · 2025-11-29T11:06:41Z

ebbbc7c -> cb156cd force push에 관하여:
69ed853 커밋메시지를 수정하였습니다.

-feat(settings): SECRET_KEY 를 환경변수에서 가져오도록 변경
+feat(app.settings): SECRET_KEY 를 환경변수에서 가져오도록 변경

Note: 기존 재귀형 타입힌트는 제대로 동작하지 않을 가능성이 있다. Context by Copilot #4: The type annotation for _JSON is incorrect. The forward reference '_JSON' in the Union is a string, but it should reference the actual type alias. This creates a recursive type definition that won't work properly.

타입 오류시에는 Django의 ImproperlyConfigured 예외가 발생하므로 따로 검사하는 로직은 추가하지 않겠습니다.

Context (by Copilot of GitHub) #4 The load_dotenv() call at module level may cause issues when settings.py imports this module. Django settings files are imported very early in the application lifecycle (in manage.py, wsgi.py, asgi.py), and calling load_dotenv() at import time means it will be executed before Django is fully initialized. This could potentially cause timing issues with environment variable loading. Consider moving this call to a function or using lazy loading, or document that .env files should be loaded before Django starts (e.g., in manage.py before importing settings).

Note: 기존 재귀형 타입힌트는 제대로 동작하지 않을 가능성이 있다. Context by Copilot #4: The type annotation for _JSON is incorrect. The forward reference '_JSON' in the Union is a string, but it should reference the actual type alias. This creates a recursive type definition that won't work properly.

Context (by Copilot of GitHub) #4 The load_dotenv() call at module level may cause issues when settings.py imports this module. Django settings files are imported very early in the application lifecycle (in manage.py, wsgi.py, asgi.py), and calling load_dotenv() at import time means it will be executed before Django is fully initialized. This could potentially cause timing issues with environment variable loading. Consider moving this call to a function or using lazy loading, or document that .env files should be loaded before Django starts (e.g., in manage.py before importing settings).

Note: 기존 재귀형 타입힌트는 제대로 동작하지 않을 가능성이 있다. Context by Copilot #4: The type annotation for _JSON is incorrect. The forward reference '_JSON' in the Union is a string, but it should reference the actual type alias. This creates a recursive type definition that won't work properly.

Context (by Copilot of GitHub) #4 The load_dotenv() call at module level may cause issues when settings.py imports this module. Django settings files are imported very early in the application lifecycle (in manage.py, wsgi.py, asgi.py), and calling load_dotenv() at import time means it will be executed before Django is fully initialized. This could potentially cause timing issues with environment variable loading. Consider moving this call to a function or using lazy loading, or document that .env files should be loaded before Django starts (e.g., in manage.py before importing settings).

Note: 기존 재귀형 타입힌트는 제대로 동작하지 않을 가능성이 있다. Context by Copilot #4: The type annotation for _JSON is incorrect. The forward reference '_JSON' in the Union is a string, but it should reference the actual type alias. This creates a recursive type definition that won't work properly.

Context (by Copilot of GitHub) #4 The load_dotenv() call at module level may cause issues when settings.py imports this module. Django settings files are imported very early in the application lifecycle (in manage.py, wsgi.py, asgi.py), and calling load_dotenv() at import time means it will be executed before Django is fully initialized. This could potentially cause timing issues with environment variable loading. Consider moving this call to a function or using lazy loading, or document that .env files should be loaded before Django starts (e.g., in manage.py before importing settings).

Note: 기존 재귀형 타입힌트는 제대로 동작하지 않을 가능성이 있다. Context by Copilot #4: The type annotation for _JSON is incorrect. The forward reference '_JSON' in the Union is a string, but it should reference the actual type alias. This creates a recursive type definition that won't work properly.

Copilot

Pull request overview

Copilot reviewed 6 out of 7 changed files in this pull request and generated 3 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-05T06:29:24Z

app/app/settings.py

+    if not ALLOWED_HOSTS:
+        raise ValueError("ALLOWED_HOSTS must be set in development")


The logic for checking ALLOWED_HOSTS in development mode is inconsistent. When DEBUG=True, an empty list for ALLOWED_HOSTS raises a ValueError, but the default value is a non-empty list ['localhost', '127.0.0.1', '[::1]']. This check will never trigger since the default is always non-empty. If the intention is to prevent users from explicitly setting an empty list via JSON, the error message should be clearer. Consider removing this check or updating it to handle the actual edge case you want to prevent.

Suggested approach:

If empty ALLOWED_HOSTS should be prevented: Check after getting the JSON value without a default

If default is sufficient: Remove the redundant check on lines 61-62

Suggested change

if not ALLOWED_HOSTS:

raise ValueError("ALLOWED_HOSTS must be set in development")

당신이 위에서 예외처리 하라면서요... ㅠㅠ

#4 (comment)

app/app/env.py

Note: 기존 재귀형 타입힌트는 제대로 동작하지 않을 가능성이 있다. Context by Copilot #4: The type annotation for _JSON is incorrect. The forward reference '_JSON' in the Union is a string, but it should reference the actual type alias. This creates a recursive type definition that won't work properly.

hepheir · 2025-12-05T06:39:06Z

진짜 마이너한 코드리뷰만 달리는 걸 보면 슬슬 LGTM이 나올 것만 같아요

Copilot

Pull request overview

Copilot reviewed 6 out of 7 changed files in this pull request and generated 3 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-05T06:44:22Z

app/app/env.py

+    Raises:
+        ValueError: 환경 변수 값이 올바른 JSON이 아니거나 required=True이고 환경 변수가 설정되지 않은 경우
+    """
+    raw_value = os.getenv(key)


[nitpick] The get_json function is missing the ability to handle the strip parameter like other getter functions. If the JSON value has leading/trailing whitespace, it could cause parsing errors. While JSON parsing typically handles whitespace, adding a strip parameter for consistency with other functions would be beneficial.

Consider adding:

def get_json(key: str, default: Optional[Any] = None, required: bool = False, strip: bool = True) -> Optional[Any]: raw_value = os.getenv(key) if raw_value is not None and strip: raw_value = raw_value.strip() # ... rest of the function

단순 consistency 때문에 get_json에 strip 인자를 추가해서 불필요한 관리포인트를 늘리는게 더 큰 코드 부채로 다가올 것 같아요.

README.md

app/app/settings.py

Copilot

Pull request overview

Copilot reviewed 6 out of 7 changed files in this pull request and generated 1 comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-05T07:07:33Z

app/app/env.py

+    if raw_value is not None:
+        try:
+            json_value = json.loads(raw_value)
+        except ValueError as e:


[nitpick] The get_json() function catches ValueError but json.loads() can also raise json.JSONDecodeError, which is a subclass of ValueError. While this works, it's more explicit and clear to catch json.JSONDecodeError specifically. This improves code readability and makes the intent clearer.

Suggested change

except ValueError as e:

except json.JSONDecodeError as e:

불필요한 예외처리 분기의 증가를 막고자 의도적으로 JSONDecodeError의 상위 클래스인 ValueError로 예외처리 한 것이므로 유지하겠습니다.

hepheir · 2025-12-05T07:09:44Z

이 정도면 충분하다고 생각합니다.
규모가 작을 것이라고 생각한 [설정에 환경변수 사용하게 변경]하는 PR이 이렇게 커질 줄이야...

hepheir · 2025-12-05T07:10:18Z

#5 가 마무리 되면, GitHub Actions 테스트도 통과할 수 있도록 조금의 구성을 더 마친 후 Merge 할 예정입니다.

… CI workflow

Copilot

Pull request overview

Copilot reviewed 6 out of 7 changed files in this pull request and generated 1 comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

.github/workflows/django.yml

Context (by Copilot of GitHub) #4 The load_dotenv() call at module level may cause issues when settings.py imports this module. Django settings files are imported very early in the application lifecycle (in manage.py, wsgi.py, asgi.py), and calling load_dotenv() at import time means it will be executed before Django is fully initialized. This could potentially cause timing issues with environment variable loading. Consider moving this call to a function or using lazy loading, or document that .env files should be loaded before Django starts (e.g., in manage.py before importing settings).

Copilot

Pull request overview

Copilot reviewed 6 out of 7 changed files in this pull request and generated 6 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-05T07:48:36Z

app/app/settings.py

+        raise ValueError(f"SECRET_KEY_FILE does not exist: {secret_key_file}")
+    if not secret_key_file.is_file():
+        raise ValueError(f"SECRET_KEY_FILE is not a file: {secret_key_file}")
+


When reading SECRET_KEY from file, there's no size limit check before calling read_text(). A malicious or misconfigured file could cause memory exhaustion. Consider adding a reasonable size check: if secret_key_file.stat().st_size > 1024: raise ValueError(f"SECRET_KEY_FILE is too large: {secret_key_file}")

Suggested change

if secret_key_file.stat().st_size > 1024:

raise ValueError(f"SECRET_KEY_FILE is too large: {secret_key_file}")

Copilot · 2025-12-05T07:48:37Z

app/app/settings.py

+    ALLOWED_HOSTS = env.get_json('ALLOWED_HOSTS',
+                                 default=['localhost', '127.0.0.1', '[::1]'])
+else:
+    ALLOWED_HOSTS = env.get_json('ALLOWED_HOSTS', required=True)


Potential security issue: In production (when DEBUG=False), if the ALLOWED_HOSTS environment variable is not set, the application will crash with a ValueError. However, there's no validation that the provided ALLOWED_HOSTS is a non-empty list. An empty JSON array [] would pass the required=True check but would make the application reject all requests. Consider adding validation to ensure ALLOWED_HOSTS is not empty in production: if not DEBUG and not ALLOWED_HOSTS: raise ValueError("ALLOWED_HOSTS cannot be empty in production")

Suggested change

ALLOWED_HOSTS = env.get_json('ALLOWED_HOSTS', required=True)

ALLOWED_HOSTS = env.get_json('ALLOWED_HOSTS', required=True)

if not ALLOWED_HOSTS:

raise ValueError("ALLOWED_HOSTS cannot be empty in production")

#4 (comment) 반복하여 제안과 번복되고 있는 내용.

app/app/env.py

Copilot

Pull request overview

Copilot reviewed 6 out of 7 changed files in this pull request and generated 3 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-05T08:02:39Z

app/app/settings.py

+        raise ValueError(f"SECRET_KEY_FILE is not a file: {secret_key_file}")
+
+    # Fail-fast, 명확한 에러 추적을 위해 파일 읽기 중 오류는 예외 처리를 하지 않음.
+    SECRET_KEY = secret_key_file.read_text(encoding='utf-8').strip()


The SECRET_KEY file is read without size validation, which could lead to memory issues if an attacker or misconfiguration points to a very large file. Consider adding a file size check before reading (e.g., if secret_key_file.stat().st_size > MAX_SECRET_KEY_SIZE: raise ValueError(...)) to prevent potential denial of service.

Copilot · 2025-12-05T08:02:39Z

app/.env.sample

+# Sample .env file for development and testing purposes
+
+# Set DEBUG to true for development, false for production
+DEBUG=false


The sample shows DEBUG=false but the comment says "Set DEBUG to true for development". This is confusing - a sample file for development should probably have DEBUG=true by default, or the comment should clarify why false is recommended even for development.

Suggested change

DEBUG=false

DEBUG=true

Copilot · 2025-12-05T08:02:39Z

app/app/settings.py

+elif secret_key:
+    SECRET_KEY = secret_key


The same minimum length validation for SECRET_KEY should also be applied when SECRET_KEY is provided directly (line 39), not just when reading from file. Consider validating SECRET_KEY length in both branches for consistency and security.

hepheir added 11 commits November 29, 2025 19:12

feat(app.env): get() - 환경변수를 가져오기위한 유틸리티 함수 작성

e9c8a3d

feat(app.env): .env 파일을 사용하여 환경변수를 불러올 수 있는 load() 함수 추가

19c0bd6

test(app.env): get() 함수에 대한 단위테스트 작성

9866419

chore: install dotenv (python-dotenv)

8d2a53f

feat(app.env): get_bool() - 환경변수로 부터 truthy, falsy 한 값을 bool 타입으로 가…

0a9a9da

…져오는 함수 추가

feat(app.settings): DEBUG 설정을 환경변수에서 가져오도록 수정

98648fa

feat(app.env): get_json() - 환경변수의 JSON 값을 파싱하여 반환하는 함수 작성

1eb40ef

test(app.env): env.get_json() 함수에 대한 단위테스트 작성

7557506

feat(app.env): get_file_content() - 환경변수에 명시된 파일에서 값을 읽어오는 함수 작성

8990a0f

test(app.env): get_file_content() 함수에 대한 단위테스트 작성

d8b0d23

feat(app.settings): SECRET_KEY가 설정되지 않은 경우 SECRET_KEY_FILE에 명시된 파…

fa942c0

…일에서 읽어오도록 기능 추가

hepheir requested a review from Copilot November 29, 2025 11:05

Copilot started reviewing on behalf of hepheir November 29, 2025 11:05 View session

hepheir force-pushed the feature/security branch from ebbbc7c to cb156cd Compare November 29, 2025 11:06

Copilot finished reviewing on behalf of hepheir November 29, 2025 11:08

This comment was marked as outdated.

Sign in to view

hepheir added 2 commits November 29, 2025 20:36

feat(app.settings): ALLOWED_HOSTS를 환경변수로 부터 받도록 수정

c9d5023

타입 오류시에는 Django의 ImproperlyConfigured 예외가 발생하므로 따로 검사하는 로직은 추가하지 않겠습니다.

hepheir force-pushed the feature/security branch from cb156cd to dae4c71 Compare November 29, 2025 17:25

hepheir force-pushed the feature/security branch from dae4c71 to 19317b8 Compare November 29, 2025 17:31

hepheir force-pushed the feature/security branch from 19317b8 to a0e5b58 Compare November 29, 2025 17:32

Copilot started reviewing on behalf of hepheir December 5, 2025 00:34 View session

Copilot finished reviewing on behalf of hepheir December 5, 2025 00:37

This comment was marked as outdated.

Sign in to view

chore: add comments for ALLOWED_HOSTS in sample .env file

2b95446

hepheir force-pushed the feature/security branch from 6af97ac to d8fd237 Compare December 5, 2025 06:24

hepheir requested a review from Copilot December 5, 2025 06:25

Copilot started reviewing on behalf of hepheir December 5, 2025 06:25 View session

Copilot finished reviewing on behalf of hepheir December 5, 2025 06:27

Copilot AI reviewed Dec 5, 2025

View reviewed changes

hepheir added 2 commits December 5, 2025 15:34

refactor(app.env): simplify truthy and falsy value checks

f62251b

test(app.env): get_bool() 함수에 대한 단위테스트 추가

af50d2a

hepheir force-pushed the feature/security branch from d8fd237 to c754c41 Compare December 5, 2025 06:38

hepheir requested a review from Copilot December 5, 2025 06:38

Copilot started reviewing on behalf of hepheir December 5, 2025 06:39 View session

Copilot AI reviewed Dec 5, 2025

View reviewed changes

hepheir added 2 commits December 5, 2025 15:58

docs: update DEBUG 설명 for 일관성 유지 "Optional" -> "선택 사항"

0ca9cdc

choe(app.settings): env.get_path()에 relative_to를 적용하지 않는 이유 명시

fff363d

Copilot AI reviewed Dec 5, 2025

View reviewed changes

ci: add environment variables for SECRET_KEY and ALLOWED_HOSTS in…

98314ea

… CI workflow

Copilot AI reviewed Dec 5, 2025

View reviewed changes

.github/workflows/django.yml Outdated Show resolved Hide resolved

hepheir added 2 commits December 5, 2025 16:40

feat(app.settings): SECRET_KEY 를 환경변수에서 가져오도록 변경

86399eb

Copilot AI reviewed Dec 5, 2025

View reviewed changes

		if not ALLOWED_HOSTS:
		raise ValueError("ALLOWED_HOSTS must be set in development")


	if secret_key_file.stat().st_size > 1024:
	raise ValueError(f"SECRET_KEY_FILE is too large: {secret_key_file}")

Conversation

hepheir commented Nov 29, 2025

Uh oh!

hepheir commented Nov 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

hepheir Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

hepheir Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

hepheir commented Dec 5, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

hepheir Dec 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

hepheir Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

hepheir commented Dec 5, 2025

Uh oh!

hepheir commented Dec 5, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

hepheir Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

hepheir commented Nov 29, 2025 •

edited

Loading

hepheir Dec 5, 2025 •

edited

Loading