Javascriptでログイン処理。教科書に載せたいレベルの悪いプログラミングの見本。全ユーザーのログイン情報も漏洩。
教科書に載せたい、つっこみポイント。
１．Javascriptによるログインの処理の大胆性
全ユーザーのデータをブラウザにダウンロードさせている。
２．SQLを直書きする危険性
悪意あるSQLを実行させることにより、全ユーザーのデータを削除できる可能性がる。
３．ユーザーのパスワードを平分で保存している問題性。
４．if (“true” === “true”) { return false; }という哲学性。
５．プログラムの１行目のコメント「TODO: put this in a different file!!!」のそこじゃない性
察するに、このコードの作者は、サーバーサイド側のプログラムを扱えない or 触る権限がない、さらにデータベースのこともあまり分かっていないために、暫定的に、JavaScript側でログイン処理を実装してしまったのだろう。そして、このような対処は自分でも良くないことは分かっているので、プログラムの１行目に「TODO: put this in a different file!!!」(最後に!が３つ並んでいるところが、緊急性を代弁している)と記入しているのだが、違うファイルだとしてもダメだ。
みんなの反応どうでしょう
あーパスワードハッシュ化されてないのね。クソコードならよくあるよねー。「"true"==="true"」？哲学かな？まぁあるよね。お、テーブル丸ごと取得してforループで探索とは漢だ。クソ度高いな。
・・まで読んでようやくこれがJavaScriptであることに気づきました。そんな馬鹿な。
SQL InjectionさせないようにSQLをパラメータで組み立ててない。優秀じゃないですか。と返答しようとしたところで、apiService.sql()の存在に気がついてしまいました。
その他の無駄なループや、true===true全員分のID/passwordをクライアントに渡していること、cookieなんて誤差だった。
JotarO_Oyanagi @JotarO_Oyanagi
リンク先の reddit にあるこのコメントwwwww
“something about if ("true" === "true") speaks to me on a spiritual level”
これがJavaScriptであるという事実で背筋が凍ったところで、
改めて1行目のコメント読むと爆笑するしか。。
お前違うファイルに持ってってもダメだろうがよ！！
クライアント側に全ユーザー情報持ってきて、その中でループさせてるのか･･･ userテーブルの全レコードが丸見えじゃないか。しかもこんなもん、他のカラムにどんな情報が入っているものやら･･･
基本ないです。クライアントサイドではあくまでログイン用のメールとpasswordをサーバに送り、サーバからは結果とかを受け取るだけです。
これ問題点はサーバサイドに自由にSQLが発行できることと、パスワードが平文で保存されてることとその他だいたい全てです。
わーお……。ネタじゃないのか。apiService.sqlが不穏すぎる。これブラウザのコンソールでapiService.sql('DELETE FROM users');ってやったらほんとに消し飛ぶのでは……。技術力から察するにDBユーザーにも全権限与えてそうだし……。
そんな大したことないやろと思って
ソースをよく見たら、
・<script> JavaScript？HTMLコードに載る可能性あり。
・謎の2連ループ 恐らく総当り 暗号化なし
恐ろしくなってきた
クライアント側でカラム指定なし全ユーザデータ読み込み
パスワード平文
where句なし
"true"==="true"の無意味if文
トリプル役満に跳満もオマケしちゃった感じ
小学生が自由研究で書いたコードかな？(白目)
Belre(五等分の花嫁非公式wiki運営) @belre_yucho
最初ぱっと見て突っ込みどころがわからなかったけど
これ下手したらシステム設計専門外の人が片手間で作ったプログラムでも同じレベルにしてしまいそうやねえ
サーバ側で実行する処理ですよね。
まさかブラウザ側で実行する処理ではないですよね。
ユーザDBへのアクセスがそこらのPCから誰でもできる、ははは、そんなことある分けないか。
上から読む人はnodejs脳、下から読む人はクライアントjQuery脳。
どちらから読んでも最初はよくある穴かと思わせて後からヤバさが一気にくる面白さ。
<!— todo: put this different file!!! —>はレビューした人がつけたのかな？笑
もしも作った本人が書いたのであれば笑うしかない
この世のあらん限りの悪を詰め込んである。これ以上の悪例が想像つかない。もはや何らかの実行時エラーで処理が途中で止まってくれてることを願うのみ。
apiService.sqlとか”true” === “true”とか色々ツッコミどころはあるけど、変数の命名とかかなりまともな部類なのではとか思ってしまった。
もっとひどいコードが世の中にたくさんあると思う
Keiichiro Miyazaki@kay_miyazaki
基本的にクソコードは自分も書いてしまう可能性があるからあまり笑えないんだが、これはちがう意味で笑えない。どういう状況でこうなったのか知りたいわ。
seletでusersの中身全部取ってきてループ回してる。where使えよ、なんてツッコミが吹き飛ぶほどに驚愕なのが、これがjavascriptてこと。
そして感動的なのがif(“true”==”true”)のスピリチュアルなコード。多分、元のコードがあって修正した結果、とんちんかんなコードになったんだと思う…思いたい。
脳みそがマヒした。
1.SQLの出来(アスターかよ)
2.そもそもSQL介さずにログインできる事。
3.SQLインジェクションのしやすさ
4.ほぼ労せずログイン1回しただけで全ユーザのアカウントとパスワードを取得できる
ある意味素晴らしい。
嘘でしょ？って言いたいが、本当なの・・・でしょうね。
・全データ取得してループ
・パスワードそのまんま
・謎のif文
・そもそもこれクライアント側・・・
うちの新人に見せてみようかな・・・。
プログラム初心者でも
for( var = i; i < acounts.length; i++)
で{ }内で何やらせたいか分かるね…
セキュリティもそうだけど、新規ユーザーほどログイン時間がかかるってアカンやんけ…
SQLiさせないようにパラメータ組み立ててないとか言ってる人いるんだがそんな馬鹿なことあるか？
DB使ってる意味がないだろ、テキストファイル全文検索と大差ないし
対策になってない気がする
大草原
逆にどういう状況だったらこれを思いついて実装に至るのかに興味がある。
ツッコミどころ多いけど、ヤバい順に
・何の認証&認可無しに自由にクエリできるエンドポイントがある
・ID/PASS全件取得して何故かクライアント側で認証
・ハッシュ化など知らん
いや、究極的には1だけで十分か？
これ中身を知ったらプログラミングを知らない人でも仰天するような有様だが、開発者を〆切で追い詰めたらこのレベルのものが納品されることは普通にあり得る。そういう代物を受け取りたくなかったら、発注者は仕様決定を先延ばしした挙句「〆切は遅らせられないから」等と言って開発者を詰めないこと。
底辺のIT土方でも恐ろしいコードなのがわかります。
userテーブルフルアクセスも怖いですけど、
IF (true == true) なんて絶っっっっっ対記述しませんからね。
わいは知っているのや、若いやつにsqlのwhere句を教えようとして、テーブル全部取得してからコードで操作すればいいから知りたくないと言ったやつが世の中には存在するんや。。。
最高にロックだった。
きっと隠し事しない正直な人が書いたコードだし、急がば回れっていう古来からの格言をリスペクトしてるし、哲学的な命題だって提示してる。
パスワードハッシュ化してないとか、なんでわざわざデータ取ってきてから検索してるんだとか、そういうのを考えたり、true==trueってなんだ？って考えた後でもっと致命的な事に気づく土曜の午後。
パスワードを使い回ししないようにすべき理由。
WEBサービスで『パスワードを暗号化してデータ保存する』のは必須ですが、暗号化していない場合運営者側にユーザーのパスワードが丸見えになります。
暗号化しているかどうかはユーザーにはわからないので注意。
こういうの具体的な指摘をしないで批判できる自信が羨ましい。
自分が気づくのだと、やばい順に
・パスワードが平文でしかも全ユーザのデータが丸見えであること
・確定情報ではないけど好きなSQLクエリを投げられそうであること
…
こんなWebサービスほんとにあるのか疑わしいけど、このWebサービス全盛の時代に「ID/PWの使い回しの怖ろしさ」「むやみやたらに会員登録することの危なさ」に改めて気付かせてくれたことは確かだよね
