Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .envrc
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
### Frontend ###
export FRONTEND_URL="http://localhost:5525"

### MinIO ###
export MINIO_ENDPOINT_URL="http://127.0.0.1:9000"
export MEDIA_BUCKET_NAME="codedang-media"
Expand Down
4 changes: 2 additions & 2 deletions apps/backend/apps/client/src/auth/auth.controller.spec.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { ConfigService } from '@nestjs/config'
import { Test, type TestingModule } from '@nestjs/testing'
import { expect } from 'chai'
import { UserService } from '@client/user/user.service'
import { AuthController } from './auth.controller'
import { AuthService } from './auth.service'

Expand All @@ -12,7 +12,7 @@ describe('AuthController', () => {
controllers: [AuthController],
providers: [
{ provide: AuthService, useValue: {} },
{ provide: UserService, useValue: {} }
{ provide: ConfigService, useValue: {} }
]
}).compile()
controller = module.get<AuthController>(AuthController)
Expand Down
19 changes: 13 additions & 6 deletions apps/backend/apps/client/src/auth/auth.controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import {
Param,
ParseEnumPipe
} from '@nestjs/common'
import { ConfigService } from '@nestjs/config'
import { AuthGuard } from '@nestjs/passport'
import { Provider } from '@prisma/client'
import { Request, Response } from 'express'
Expand All @@ -27,7 +28,10 @@ import type { GithubUser, KakaoUser } from './interface/social-user.interface'

@Controller('auth')
export class AuthController {
constructor(private readonly authService: AuthService) {}
constructor(
private readonly authService: AuthService,
private readonly configService: ConfigService
) {}

setJwtResponse = (res: Response, jwtTokens: JwtTokens) => {
res.setHeader('authorization', `Bearer ${jwtTokens.accessToken}`)
Expand Down Expand Up @@ -158,15 +162,18 @@ export class AuthController {
@AuthNotNeededIfPublic()
@Get('kakao-callback')
@UseGuards(AuthGuard('kakao'))
async kakaoLogin(
@Res({ passthrough: true }) res: Response,
@Req() req: Request
) {
async kakaoLogin(@Res() res: Response, @Req() req: Request) {
const kakaoUser = req.user as KakaoUser
const result = await this.authService.kakaoLogin(kakaoUser)
const frontendUrl = this.configService.getOrThrow('FRONTEND_URL')

if ('oauthToken' in result) return result
if ('oauthToken' in result) {
return res.redirect(
`${frontendUrl}/login?modal=social-unlinked&oauthToken=${result.oauthToken}`

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

oauthToken이 URL에 포함되어 있으면 브라우저 히스토리에 기록이 남으니 보안에 안좋을거 같아서
쿠키에 저장하고, 리다이렉트 후 프론트에서 쿠키를 읽는 방식은 어떨까요?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

원래는 쿠키를 통해서 이를 전달하려고 했는데, 다음 이유 때문에 Query String으로 바꾸었어요.

FE에서 쿠키에 저장된 값을 직접 읽어서 사용하려면 httpOnly 값을 false로 두어야 하는데 그러면 XSS 공격에 노출될 수 있어요. 이럴 경우에는 사실 상 QueryString으로 전달하는 것이랑 크게 다른게 없지 않나 싶어서요.

또, 쿠키로 전달하면 브라우저 내부에 쿠키가 지속적으로 저장될 텐데, 회원가입이나 소셜 연동 이후에 FE 쪽에서 쿠키 무효화까지 신경써줘야 할 것 같은데, 그럴 경우에는 품이 크게 들 수 있을 것 같아요.

추가적으로, oauthToken 내부에 사실 provider 값과 provider에서 제공하는 oauthId 정도인데 이게 보안 상 그렇게 중요한가? 싶기도 해요

)
}
Comment on lines +170 to +174

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

하드코딩된 문자열 템플릿 대신 URL 객체를 사용하여 리다이렉트 URL을 생성하는 것이 좋습니다. URL 객체를 사용하면 frontendUrl에 슬래시(/)가 포함되어 있는지 여부와 상관없이 경로를 안전하게 병합할 수 있으며, searchParams를 통해 oauthToken과 같은 쿼리 파라미터를 설정하면 자동으로 안전하게 URL 인코딩이 적용됩니다.

Suggested change
if ('oauthToken' in result) {
return res.redirect(
`${frontendUrl}/login?modal=social-unlinked&oauthToken=${result.oauthToken}`
)
}
if ('oauthToken' in result) {
const redirectUrl = new URL('/login', frontendUrl)
redirectUrl.searchParams.set('modal', 'social-unlinked')
redirectUrl.searchParams.set('oauthToken', result.oauthToken)
return res.redirect(redirectUrl.toString())
}
References
  1. When handling social authentication, do not trust client-provided OAuth IDs directly. Instead, use a server-signed token (e.g., JWT) to securely pass OAuth information for server-side verification before linking accounts.


this.setJwtResponse(res, result.jwtTokens)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

리다이렉트(302 Redirect) 응답에 authorization 헤더를 설정하더라도, 브라우저가 리다이렉트를 자동으로 처리하기 때문에 프론트엔드 애플리케이션에서는 이 헤더 값을 읽을 수 없습니다.

따라서 setJwtResponse 내에서 설정되는 res.setHeader('authorization', ...)를 통한 accessToken 전달은 무효화됩니다.

프론트엔드가 로그인 완료 후 refresh_token 쿠키를 이용해 /reissue를 호출하여 accessToken을 획득하는 구조라면 문제가 없으나, 그렇지 않고 콜백 응답에서 직접 accessToken을 받아야 하는 구조라면 토큰 전달 방식을 재검토해야 합니다.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

이거 제미나이 코멘트도 지금 코드상으로는 반영해야 할거 같아요! 나머지 medium priority 2개는 흠.. 안해도 괜찮을거 같은데 잘 모르겠네요

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

기존 가입자가 소셜 로그인 (소셜 로그인 후에 바로 메인 페이지로 가능 케이스)
BE에서 res.redirect('/')를 사용해서 리다이렉트하면 refresh_token은 쿠키에 저장되지만 access_token은 헤더에 들어가는 값이라 자연스럽게 소멸되어요.
이런 경우에는 로그인 후 메인 페이지로 갔지만 로그인 정보가 없는 상황(액세스 토큰 정보가 없음)이라서 FE에서 별도로 api 서버로 reissue를 호출해야 할 것 같아요.

위 쪽 코멘트 통해서 FE쪽에 이미 요청드렸습니다! 저희 쪽에서는 처리가 힘든 문제라서요

return res.redirect(`${frontendUrl}/`)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

frontendUrl 뒤에 슬래시(/)가 중복으로 붙거나 누락되는 문제를 방지하기 위해 URL 객체를 사용하여 리다이렉트 URL을 안전하게 생성하는 것을 권장합니다.

    const redirectUrl = new URL('/', frontendUrl)
    return res.redirect(redirectUrl.toString())

}
}
Loading