Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add game pin generation functionality #12

Closed
wants to merge 2 commits into from
Closed

Add game pin generation functionality #12

wants to merge 2 commits into from

Conversation

mmpotulo28
Copy link
Member

No description provided.

Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey @mmpotulo28 - I've reviewed your changes and they look great!

Here's what I looked at during the review
  • 🟡 General issues: 2 issues found
  • 🟡 Security: 1 issue found
  • 🟢 Testing: all looks good
  • 🟢 Complexity: all looks good
  • 🟢 Docstrings: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.

Comment on lines +1 to +14
function generatePIN() {
let digits = Math.floor(1000 + Math.random() * 9000);

let letters = '';
const alphabet = 'ABCDEFGHIJKLMNOPQRSTUVWXYZ';
for (let i = 0; i < 3; i++) {
letters += alphabet.charAt(Math.floor(Math.random() * alphabet.length));
}

let pin = digits + letters;

document.getElementById('pinDisplay').value = pin;
sessionStorage.setItem('gamePin', pin);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

🚨 suggestion (security): Consider using a more secure random generator for PINs.

The current method using Math.random is not cryptographically secure, which might be a concern for the application's security requirements.

pages/auth/gamepin/gamepin.js Outdated Show resolved Hide resolved
}

window.onload = function () {
let storedPin = sessionStorage.getItem('gamePIN');
Copy link
Contributor

Choose a reason for hiding this comment

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

issue (bug_risk): Mismatch in case sensitivity for sessionStorage key.

The key 'gamePIN' used in getItem does not match the case of 'gamePin' used in setItem, which could lead to unexpected behavior.

Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
Copy link

cloudflare-pages bot commented Apr 4, 2024

Deploying devpost-hackathon with  Cloudflare Pages  Cloudflare Pages

Latest commit: 8a60b6a
Status: ✅  Deploy successful!
Preview URL: https://5b148018.devpost-hackathon.pages.dev
Branch Preview URL: https://sam.devpost-hackathon.pages.dev

View logs

Copy link
Member Author

@mmpotulo28 mmpotulo28 left a comment

Choose a reason for hiding this comment

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

@samkeleN address the changes suggested by the AI

issue (bug_risk): Mismatch in case sensitivity for sessionStorage key.

The key 'gamePIN' used in getItem does not match the case of 'gamePin' used in setItem, which could lead to unexpected behavior.

Copy link
Collaborator

@Helipterum Helipterum left a comment

Choose a reason for hiding this comment

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

@samkeleN

Utilize the window.crypto.getRandomValues() method to generate cryptographically secure random bytes if necessary.

@mmpotulo28
Copy link
Member Author

reopen this PR after you made the requested changes

@mmpotulo28 mmpotulo28 closed this Apr 5, 2024
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.

None yet

2 participants