Skip to content

Conversation

@hepheir
Copy link
Member

@hepheir hepheir commented Nov 29, 2025

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

@hepheir
Copy link
Member Author

hepheir commented Nov 29, 2025

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

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

This comment was marked as outdated.

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 예외가 발생하므로 따로 검사하는 로직은 추가하지 않겠습니다.
hepheir added a commit that referenced this pull request Nov 29, 2025
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).
hepheir added a commit that referenced this pull request Nov 29, 2025
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 added a commit that referenced this pull request Nov 29, 2025
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).
hepheir added a commit that referenced this pull request Nov 29, 2025
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).
hepheir added a commit that referenced this pull request Nov 29, 2025
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 added a commit that referenced this pull request Nov 29, 2025
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).
hepheir added a commit that referenced this pull request Nov 29, 2025
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).
hepheir added a commit that referenced this pull request Nov 29, 2025
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.
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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:

Copilot uses AI. Check for mistakes.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@hepheir
Copy link
Member Author

hepheir commented Dec 5, 2025

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

@hepheir
Copy link
Member Author

hepheir commented Dec 5, 2025

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

hepheir added a commit that referenced this pull request Dec 5, 2025
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).
hepheir added a commit that referenced this pull request Dec 5, 2025
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.
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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).
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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}")

Copy link

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 uses AI. Check for mistakes.
ALLOWED_HOSTS = env.get_json('ALLOWED_HOSTS',
default=['localhost', '127.0.0.1', '[::1]'])
else:
ALLOWED_HOSTS = env.get_json('ALLOWED_HOSTS', required=True)
Copy link

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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")

Copilot uses AI. Check for mistakes.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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()
Copy link

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 uses AI. Check for mistakes.
# Sample .env file for development and testing purposes

# Set DEBUG to true for development, false for production
DEBUG=false
Copy link

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 uses AI. Check for mistakes.
Comment on lines +38 to +39
elif secret_key:
SECRET_KEY = secret_key
Copy link

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
@hepheir hepheir merged commit 0d721c3 into develop Dec 5, 2025
17 checks passed
@hepheir hepheir deleted the feature/security branch December 5, 2025 08:04
@hepheir hepheir self-assigned this Dec 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants