Revamp for PHP 8.x and Magento 2 #13
Conversation
|
Hey sorry for the wait, but I didn't get any notifications about this :( Did you already try to run it? |
|
Hey!.. no worries.. yes, there shouldn't be a problem to run it in docker if you follow https://github.com/napoly/docker-monero-magento/tree/monero-integration?tab=readme-ov-file#manual-setup |
Controller/Gateway/MoneroPayment.php
Outdated
| $xmr_subaddress = $this->monero_daemon->create_address(0); | ||
| setcookie('xmr_subaddress', $xmr_subaddress['address'], time() + 2700); | ||
| } else { | ||
| $xmr_subaddress = ['address' => $_COOKIE['xmr_subaddress']]; |
There was a problem hiding this comment.
Parse the address before set it to $xmr_subaddress otherwise it's an input that might be controlled by an attacker (nothing too sensitive, but..)
There was a problem hiding this comment.
sanitized and validated.. pls check
| $address_index = $address_index['index']['minor']; | ||
| } | ||
| else { | ||
| return $txs; |
There was a problem hiding this comment.
there's something odd here..
Write
if(!isset(...)){
return $txs;
}
and move the other branch after that check :) better readability
| $txs = $this->check_payment_rpc($payment_id); | ||
|
|
||
| // If num_confirmations is 0, simply check if payment has been received | ||
| if ($num_confirmations == 0) { |
There was a problem hiding this comment.
Good, I'm thinking about we might want to show something in the settings if num_confirmations == 0. Also, I don't see any check for num_confirmations < 0. Are we sure nobody can actually set that value?
There was a problem hiding this comment.
added more info and validation validate-zero-or-greater so it looks like:
Controller/Gateway/MoneroPayment.php
Outdated
| if(isset($_GET['first'])){ | ||
|
|
||
| if (isset($_GET['first'])) { | ||
| $first = $_GET['first']; |
There was a problem hiding this comment.
Thinking that we might want to parse and validate all the fields..
There was a problem hiding this comment.
Thanks for the suggestion! I’ve added some basic sanitation for the fields to address potential issues. However, I believe this section might not even be necessary, as the data is already provided and validated in the previous step. Not really part of the bounty.. just fyi
- adds number of confirmation ability - switches to sub-addresses - added zero unlock time for tx
napoly
force-pushed
the
revamp-magento2
branch
from
3c4a25d to
fe01d21
Compare
|
Thanks! It's still a very rough MVP, but it would be great to hook it up on Packagist so folks can try it out and provide feedback. |
should resolve: #12
For a live demonstration that can be tested, please run a Monero wallet RPC locally (host.docker.internal) and follow the manual setup instructions for Dockerized Magento available at https://github.com/napoly/docker-monero-magento/tree/monero-integration.